-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add option to use weval
, an experimental JS AOT option using partial evaluation.
#541
Conversation
@@ -4,11 +4,12 @@ import { mkdir, readFile } from "node:fs/promises"; | |||
import { isFile } from "./isFile.js"; | |||
import { isFileOrDoesNotExist } from "./isFileOrDoesNotExist.js"; | |||
import wizer from "@bytecodealliance/wizer"; | |||
import weval from "@cfallin/weval"; |
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.
A standard technique for lazy initialization in JavaScript would be to turn this into a dynamic import()
nested into the async code path where Weval is used only.
@cfallin, as this is from a fork, our CI won't work correctly as it will not allow access to the GitHub Secrets which are required for the full test suite to run I've branched from your fork and made patches to the GitHub Workflows/Jobs, which is available here -- the latest CI run is showing that some of the web-platform-test results have changed when using the weval build. To run the web-platform-tests locally and view the results, you can do the following from the project root:
To make it simpler to see the difference test results, I've also committed and pushed the web-platform-test results which were updated when using the weval build -- 64ffa2f18597117302d2696138df97aac2ccd890 |
@JakeChampion thanks, I'm able to reproduce this locally and I'm taking a look! |
After a lot of debugging, I did the obvious thing and tried repo-version combinations: reverting the |
@JakeChampion actually, after reverting to this repo's
I might need your help to dig more deeply! |
…l evaluation. This PR pulls in some still-experimental work to partially evaluate the SpiderMonkey JS interpreter together with JS scripts, ahead-of-time, to produce compiled code. It should not yet be considered production-worthy, pending more testing and proving out its stability and performance; but, it is at a point where we believe it should be under a *non-default* option so that users of the SDK may try it. No guarantees! It may explode your bytecode! The support works by bundling two builds of the engine Wasm -- with and without the weval intrinsics -- and invoking either Wizer with the "normal" build or weval with the "weval" build, as appropriate. The `--enable-weval` option selects the latter.
939d937
to
1473a87
Compare
This has been superseded by #883 with a new interpreter (PBL) processed by weval; closing this PR! |
This PR pulls in some still-experimental work to partially evaluate the SpiderMonkey JS interpreter together with JS scripts, ahead-of-time, to produce compiled code.
It should not yet be considered production-worthy, pending more testing and proving out its stability and performance; but, it is at a point where we believe it should be under a non-default option so that users of the SDK may try it. No guarantees! It may explode your bytecode!
The support works by bundling two builds of the engine Wasm -- with and without the weval intrinsics -- and invoking either Wizer with the "normal" build or weval with the "weval" build, as appropriate. The
--enable-weval
option selects the latter.I have measured weval showing speedups of ~30% on a Markdown renderer on my machine, and up to ~3.5x on the
crypto
benchmark in the Octane suite (1.83x speedup geomean).This refers to a hash for the
runtime/spidermonkey
submodule that is in fastly/spidermonkey-wasi-embedding#17; that will need to be merged first (and probably bytecodealliance/gecko-dev#5 before that).I'm not entirely sure about some of the build/packaging integration; that's the bulk of the changes here and will need special review.
The package deps include
@cfallin/weval
; if it's preferable, I should probably move the repo to BA and get the appropriate permissions to publish it as@bytecodealliance/weval
.