-
Notifications
You must be signed in to change notification settings - Fork 6
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
@@ -3,33 +3,44 @@ | |||
set -euo pipefail | |||
set -x | |||
|
|||
if (wasm-opt --version | grep -q "wasm-opt version" ); then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.