Improve Wasm test builds #2181

brian-carroll · 2021-12-10T22:11:01Z

Two improvements:

Fix a linking issue with roc_alloc:
Until now, we have only needed roc_alloc to exist, we never called it because gen_wasm hadn't implemented anything with heap allocation. But in Wasm recursive tags #2169 I ran into an issue.
In tests, roc_alloc is just a thin wrapper around libc malloc. I was creating it via code generation in the same Wasm module as the test code. But that means it's defined as an internal symbol! And in a real app, roc_alloc needs to be an external symbol.
This PR deletes the code-generated version. Instead we put the platform code in a .c file that gets compiled separately and linked with our test code. This is more similar to the real use case.
Generate Wasm libc.a from a build script instead of checking it in to git.
Until now we've has the Wasm libc as a binary checked into the repo. It was hard to figure out how to make it a build artifact.
But a few weeks ago @Luukdegram suggested a way, and I've implemented that here.
In the test_gen build script I compile a dummy C program and tell Zig to put its cache files in a build directory where I know there will be nothing else called libc.a. Then I can easily find it afterwards.

One downside:

For some reason, the tests are now slower. On my machine, around 43 seconds instead of 28.
Now that the platform code is external, we are linking 4 files instead of 3, maybe that's a factor?
libc is not getting rebuilt on each test. I checked.

brian-carroll · 2021-12-11T09:46:29Z

@bhansconnect this might be interesting for you. I think both of these issues also apply to gen_dev.

gen_dev also inserts roc_alloc into the test binary so it might run into the same issue with internal vs external symbols.
Also if we eventually start running ARM tests in CI, we could use the same technique to build a libc.a for ARM on an x86 host.

folkertdev · 2021-12-11T14:46:28Z

compiler/test_gen/build.rs

+fn build_wasm_libc(out_dir: &str) {
+    let source_path = "src/helpers/dummy_libc_program.c";
+    println!("cargo:rerun-if-changed={}", source_path);
+    let cwd = Path::new(".");
+    let zig_cache_dir = format!("{}/zig-cache-wasm32", out_dir);
+
+    run_command(
+        cwd,
+        "zig",
+        [
+            "build-exe", // must be an executable or it won't compile libc
+            "-target",
+            "wasm32-wasi",
+            "-lc",
+            source_path,
+            "-femit-bin=/dev/null",
+            "--global-cache-dir",
+            &zig_cache_dir,
+        ],
+    );
+
+    let libc_path = run_command(cwd, "find", [&zig_cache_dir, "-name", "libc.a"]);
+
+    println!("cargo:rustc-env={}={}", LIBC_PATH_VAR, libc_path);
+}


I tried adding

"-O", "ReleaseSafe",

here and it makes the tests much (10x) slower. So it must be doing something while actually running the tests. Maybe there is more to link? even so that is an indication that we could maybe speed things up by making tweaks here

There's something seriously weird happening. I enabled ReleaseSafe as you suggested and it made no difference. Then I commented it out again and it got 10x slower... It's as if it was using the previous version or something.

So there are at least two weird things happening

Why is Cargo running the build script on every test?

Why is it running a previous version?

so I wonder if we can break this up into parts: 1) using the libc.a that this build.rs generates, 2) use the platform code that this build.rs generates, and 3) using build.rs to generate those files on the fly

I think that should be as simple as running this build.rs, finding out where the binaries are stored, copy them somewhere safe, then hardcode their paths and remove the build.rs entirely.

Why is Cargo running the build script on every test?

I might be jumping to conclusions on this. I'll keep digging.

it's an explanation that would fit with what we're seeing, but I tried compiling with passing -vv (extra verbosity) to cargo and you can see some printlns for the env vars on the first compile, but nothing after that

Thank you both for the information, exactly what I needed.

@brian-carroll The reason why you weren't seeing any results using zig build-exe is due to wasi's reactor/command model. Zig implements this and you can override which model to use using the -mexec-model=<value> flag. The idea behind this model is to tell the runtime how to start or initialize the module. Command is similar to an executable, in that it exports a _start symbol which is called by the runtime to start the module.
Reactor exports an _initialize symbol which allows you to initialize data on startup.

The default Zig uses is Command, which means it must export a _start symbol. Considering you're using the --no-entry flag, I assume there's no start function required. This means that Zig will export its own _start function instead. Which essentially calls a program's main, and then calls wasi's exit 'syscall'. (https://github.com/ziglang/zig/blob/master/lib/std/start.zig#L218-L225).

Now we have the background information out of the way, let's get back to the main goal here: Linking multiple object files with libc into a final wasm binary.
To achieve this, in my opinion, the following command should be used:
zig build-lib app.o builtins.o test_platform.o -lc -dynamic -femit-bin=final.wasm
This creates a wasm binary that dynamically links libc.
Currently, Zig already appends a few flags by default when using build-lib. These are --alow-undefined, --no-entry, --export-all (https://github.com/ziglang/zig/blob/master/src/link/Wasm.zig#L1130-L1137). This means it should already support the use cases above.

As you may notice, there's 1 big problem with this: --export-all. This will export all public symbols, which is probably not what you intended to do, however, status quo should already work. My idea is to make a PR in the Zig repo to support both the --export and --export-dynamic flags, that will override the default behavior of exporting all symbols. I'll make this PR within the next few days in the hope it's on time for the 0.9.0 release (20th of December), so you could consider this approach if Roc were to switch to 0.9.0.
I just want to stress that this was just a suggestion, in no way am I trying to tell you to use one approach over the other :)

EDIT: Made a PR to support those flags: ziglang/zig#10320

Thanks @Luukdegram !

Sorry for the slow reply, I'm terrible at checking GitHub notifications!
Next time I'm looking into build stuff, I'll come back and try your recommendations. (That might be fairly soon since Zig 0.9 is coming out.)
I see your PR got merged, which is great!
You're right, we would definitely not want to include all symbols! I accidentally increased our Wasm test time by 50% just by including fprintf in the binary, so if all of libc and compiler_rt were in there... 😱

If I use dynamic linking, does that mean it won't actually insert the libc code into my final.wasm binary? If so... how do I get that code? Where would I dynamically link it from?

If I use dynamic linking, does that mean it won't actually insert the libc code into my final.wasm binary? If so... how do I get that code? Where would I dynamically link it from?

I think the name is currently a little bit misleading because it isn't dynamic linking in the same sense of what you're used to in languages like C. (There's a real dynamic linking idea for wasm in the works, but that's not stable yet. But that's for another rainy day). Right now static will just make an archive object file, whereas passing -dynamic will result in a regular .wasm file, but we only support linking with libc in the latter.

tl;dr: The code (libc) actually does get inserted into the final wasm binary.

Oh I see. Thanks for explaining!

folkertdev · 2021-12-11T21:48:21Z

compiler/test_gen/src/helpers/wasm.rs

-std::thread_local! {
-    static TEST_COUNTER: Cell<u32> = Cell::new(0);
-}
-


not sure where this comes from? it was unused

Yeah I have no idea where that came from either, I'd say it's been sitting there unused for a long time.

brian-carroll mentioned this pull request Dec 10, 2021

Wasm recursive tags #2169

Merged

brian-carroll force-pushed the wasm-test-platform branch 2 times, most recently from ab52cb5 to faf9d8b Compare December 11, 2021 09:36

brian-carroll added 6 commits December 11, 2021 10:15

Build a Wasm test platform in test_gen

53d2214

Build a Wasm libc.a from test_gen/build.rs

aefc6e7

Update Wasm tests to use libc and platform built by build.rs

2d4dfb4

Delete libc.a from gen_wasm

0fe8575

Only run Wasm build steps for test_gen when gen-wasm feature is enabled

544caf0

Edit dummy libc program

e9343d8

brian-carroll force-pushed the wasm-test-platform branch from faf9d8b to e9343d8 Compare December 11, 2021 10:15

folkertdev reviewed Dec 11, 2021

View reviewed changes

brian-carroll and others added 2 commits December 11, 2021 20:13

Remove fprintf from compilation for faster test runs

11118bb

use env! macro

99d12c4

folkertdev reviewed Dec 11, 2021

View reviewed changes

folkertdev approved these changes Dec 11, 2021

View reviewed changes

folkertdev merged commit b9395e9 into trunk Dec 11, 2021

folkertdev deleted the wasm-test-platform branch December 11, 2021 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Wasm test builds #2181

Improve Wasm test builds #2181

brian-carroll commented Dec 10, 2021 •

edited

Loading

brian-carroll commented Dec 11, 2021

folkertdev Dec 11, 2021

brian-carroll Dec 11, 2021

folkertdev Dec 11, 2021

brian-carroll Dec 11, 2021

folkertdev Dec 11, 2021

Luukdegram Dec 12, 2021 •

edited

Loading

brian-carroll Dec 18, 2021

brian-carroll Dec 18, 2021

Luukdegram Dec 23, 2021

brian-carroll Dec 23, 2021

folkertdev Dec 11, 2021

brian-carroll Dec 12, 2021

Improve Wasm test builds #2181

Improve Wasm test builds #2181

Conversation

brian-carroll commented Dec 10, 2021 • edited Loading

brian-carroll commented Dec 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luukdegram Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-carroll commented Dec 10, 2021 •

edited

Loading

Luukdegram Dec 12, 2021 •

edited

Loading