Pull in weval branch of SpiderMonkey and add build. #17
Conversation
This pulls in a new bytecodealliance/gecko-dev commit hash with weval support, and produces a new separate build of the engine with intrinsics added so that we can apply the [weval](https://github.com/cfallin/weval) tool to obtain speedups with AOT compilation/specialization. This is a re-do of fastly#5, but with fixes from bytecodealliance/gecko-dev#5 applied.
build-engine.sh
Outdated
| set -euo pipefail | ||
| set -x | ||
|
|
||
| if (wasm-opt --version | grep -q "wasm-opt version" ); then |
There was a problem hiding this comment.
Our js-compute-runtime build still assumes wasm-opt in path, so I think this may cause conflicts?
@elliottt maybe we should take an approach in js-compute-runtime of using different pathing finally here?
There was a problem hiding this comment.
This check actually fails if you don't have wasm-opt in PATH, so it seems like it must be set to /bin/true for this to pass?
There was a problem hiding this comment.
Ah, this is the confusing nature of shell scripting: it's meant to fail (and does fail) if we do have wasm-opt in $PATH. The subshell (in parens) returns exit code 1 (failure) if wasm-opt doesn't exist or grep fails to find the text (and returns its own failure code). The if in the parent shell takes 0 as truthy, so this is saying "if success (wasm-opt exists and is real wasm-opt), then fail with message".
To the broader point of wasm-opt existing for the rest of the build... this indicates to me that perhaps the better approach is to actively mask wasm-opt with a wrapper and put that at the front of PATH for this build. I'll make that change; it's more general anyway!
There was a problem hiding this comment.
How about this instead?
| if (wasm-opt --version | grep -q "wasm-opt version" ); then | |
| if command -v wasm-opt > /dev/null; then |
There was a problem hiding this comment.
The js-compute-runtime does use wasm-opt to strip builds, this might make it difficult to run both builds in the same machine. We worked around the use of wasm-opt by llvm by temporarily putting a shell script that exits immediately in the path earlier than the real wasm-opt. Would that be an option here?
You suggested this in an earlier comment, and I missed it completely :)
There was a problem hiding this comment.
I've got some edits to this branch, in a different branch https://github.com/fastly/spidermonkey-wasi-embedding/tree/jake/weval-edits which has a similar change but only for CI and not the build.sh script itself
There was a problem hiding this comment.
Cool, I pulled in your updates to the gitignore list @JakeChampion and I've pushed an update that shadows wasm-opt with its own fake-bin directory in PATH. This avoids the need to move it out of the way during the CI build. I also went ahead and merged rebuild.sh with build-engine.sh (former calls later with an arg) because it was getting a little too unwieldy to duplicate logic.
This pulls in a new bytecodealliance/gecko-dev commit hash with weval support, and produces a new separate build of the engine with intrinsics added so that we can apply the weval tool to obtain speedups with AOT compilation/specialization.
This is a re-do of #5, but with fixes from bytecodealliance/gecko-dev#5 applied.