Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Upgrade RTS Dependencies #4677

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

luc-blaeser
Copy link
Contributor

@luc-blaeser luc-blaeser commented Aug 29, 2024

Upgrading Motoko RTS dependencies:

  • LLVM 18.1.8
  • Rust 1.82.0

Special aspects:

  • Support new dylink.0 instead of dylink.
  • Support GOT.mem for RTS in the Motoko linker.
  • Adjusting the RTS code in line with the latest Rust features/rules.
  • No longer needing emscripten: Use custom Rust targets .
  • Get rid of emscripten by using custom Rust targets for generating PIC-relocatable Wasm.
  • Get rid of musl by using Rust builtins instead.
  • Float formatting changes due to removal of musl: The output format of NaN and infinity has changed. Moreover, one particular output format (hexadecimal float) is no longer supported. Updating the documentation and base library is needed.
  • Optimization: Use bulk-memory operations because memcpy of Rust is slower than musl.

Note:

  • If your nix-shell crashes with a segmentation fault with this PR, you need to upgrade nix. For this, you need to manually uninstall and re-install nix.

TODO:

@luc-blaeser luc-blaeser self-assigned this Aug 29, 2024
@luc-blaeser luc-blaeser added the dependencies Pull requests that update a dependency file label Aug 29, 2024
Copy link

github-actions bot commented Aug 29, 2024

Comparing from b7fa639 to 6572974:
In terms of gas, 3 tests regressed, 2 tests improved and the mean change is -0.0%.
In terms of size, 5 tests improved and the mean change is -1.6%.

rts/Makefile Outdated Show resolved Hide resolved
luc-blaeser and others added 4 commits October 8, 2024 14:44
* Patch Rust for shared Wasm libraries

* Get rid of `musl`

* Further simplify Wasm linking

* Update benchmark results

* No longer need `wasm-ld` patch

* Update test result

* Add distinction for Linux Ocaml build

* Revert "No longer need `wasm-ld` patch"

This reverts commit 3d0f34e.

* Revert "Update test result"

This reverts commit 8fc5c08.

* Use patched wasm-ld during Rust build

* Table offset must be at least 1 for Rust RTS

* Adjust linker test result

* Use bulk memory operations

* Do not need `wasm-lld` patch with shared targets

* Adjust expected test result

* Adjust nix build script

* Fix nix build script
@luc-blaeser
Copy link
Contributor Author

Size compression is no longer needed for the specific Rust target backends.
Instead, we can again optimize for speed:

This is the latest perf diff on CI (locally repeated):

Comparing from b7fa63936cf708952deae0dd0b0429faa1baa119 to a10ab53a1ec3e5e48dbce36d6c7dd8547503c85c:
In terms of gas, 3 tests regressed, 2 tests improved and the mean change is -0.0%.
In terms of size, 5 tests improved and the mean change is -1.6%.

@luc-blaeser
Copy link
Contributor Author

According to the GC benchmark, it seems that this upgrade/cleanup even improves performance:

Mode Before Now Improvement
Classical (copying GC) 2.33e10 2.29e10 1.7%
Classical (incremental GC) 2.39e10 2.35e10 1.7%
EOP 2.54e0 2.50e10 1.6%

Comment on lines -73 to 82
"branch": "next-moc",
"branch": "pull/589/head",
"description": "The Motoko base library",
"homepage": "",
"owner": "dfinity",
"repo": "motoko-base",
"rev": "21a80ab250501a5de3365114abb242559573fecc",
"sha256": "0zm60azafdfczdiqkqs1fwgxf3crb2bk0vs7y09rvnwkqgqkxxw8",
"rev": "1bece926bfc417db5661c66d8617f6b09ab06b95",
"sha256": "02i8s4ybjmbbf72jclpvqcsxh4dypgsxqsagiga64jqyprmqzvlr",
"type": "tarball",
"url": "https://github.com/dfinity/motoko-base/archive/21a80ab250501a5de3365114abb242559573fecc.tar.gz",
"url": "https://github.com/dfinity/motoko-base/archive/1bece926bfc417db5661c66d8617f6b09ab06b95.tar.gz",
"url_template": "https://github.com/<owner>/<repo>/archive/<rev>.tar.gz"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to be changed back to next-moc once dfinity/motoko-base#589 is merged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not forget this

@@ -78,7 +78,7 @@ let
};
};

# No testing of atdgen, as it pulls in python stuff, tricky on musl
# No testing of atdgen, as it pulls in python stuff
atdgen = super.ocamlPackages.atdgen.overrideAttrs { doCheck = false; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be doCheck = true now?

Copy link
Contributor

@ggreif ggreif Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or rather leave it uncustomized?

in rec {
rustc-nightly = rust-channel.rust.override {
targets = [
"wasm32-unknown-emscripten"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, why were these here?

# pulls in clang (wrapped) and clang-13 (unwrapped)
llvmPackages_13.clang
# pulls in wasm-ld
llvmPackages_13.lld
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this package now merged into bintools? We do use wasm-ld-18 below...


use motoko_rts_macros::{classical_persistence, enhanced_orthogonal_persistence};
// The meaning of the `mode` parameter is documented in motoko-base, function Float.format()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice move!

Comment on lines +759 to +760
List.filter (fun import -> not (is_got import.it.module_name)) imports

Copy link
Contributor

@ggreif ggreif Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
List.filter (fun import -> not (is_got import.it.module_name)) imports
List.filter (fun import -> not (is_got import.it.module_name)) imports

added newline removed

@@ -915,7 +916,7 @@ let encode (em : extended_module) =
patch s (p + 1) (lsb (n lsr 8));
patch s (p + 2) (lsb (n lsr 16));
patch s (p + 3) (lsb (n lsr 24))
let dw_patches = ref Fun.id
let dw_patches = ref (fun i -> i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh, why this?

Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Heroic work on the dylink0 stuff. Being stuck on clang-13 gave me nightmares...
Nice to see the cycle improvements too. Getting rid of musl means one less wheel to maintain.

Good work! Added a few Qs...

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Not sure I fully understand the intricacies so 🤞

Comment on lines -73 to 82
"branch": "next-moc",
"branch": "pull/589/head",
"description": "The Motoko base library",
"homepage": "",
"owner": "dfinity",
"repo": "motoko-base",
"rev": "21a80ab250501a5de3365114abb242559573fecc",
"sha256": "0zm60azafdfczdiqkqs1fwgxf3crb2bk0vs7y09rvnwkqgqkxxw8",
"rev": "1bece926bfc417db5661c66d8617f6b09ab06b95",
"sha256": "02i8s4ybjmbbf72jclpvqcsxh4dypgsxqsagiga64jqyprmqzvlr",
"type": "tarball",
"url": "https://github.com/dfinity/motoko-base/archive/21a80ab250501a5de3365114abb242559573fecc.tar.gz",
"url": "https://github.com/dfinity/motoko-base/archive/1bece926bfc417db5661c66d8617f6b09ab06b95.tar.gz",
"url_template": "https://github.com/<owner>/<repo>/archive/<rev>.tar.gz"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not forget this

@@ -41,33 +40,3 @@ unsafe impl GlobalAlloc for EphemeralAllocator {

#[global_allocator]
static ALLOCATOR: EphemeralAllocator = EphemeralAllocator;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this stuff need for musl and that's why we can remove it?

if imports = [] then
m
(* Computes the absolute address of the GOT.mem data pointer *)
let data_pointer exported_global_index = if uses_memory64 m then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that you should do this now, but I wonder if it would be possible to just bump the (mutable?) globals by the offset using an initialization function, rather than recomputing the absolute address at each read. Another advantage, apart from perf, would be that any debug info would remain valid (rather than disturbing the instruction offset in the code.

For each `GOT.mem`, we replace the GOT accesses in the AST by code that computes
the absolute address of the data. This is the library's memory base plus the
relative data offset stored in the global as determined in the first step.
The `GOT.mem` import is replaced by a dummy global that only serves to maintain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the dummy global could instead hold the absolute/resolved address, and we replace references to the import by references to the resolved global? But I don't fully understand this stuff..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants