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

Remove usage of the custom rquickjs fork #675

Merged

Conversation

saulecabrera
Copy link
Member

@saulecabrera saulecabrera commented Jun 27, 2024

This commit deprecates the usage of the custom rquickjs fork and pins the rquickjs version to 0.6.1, which is the current version in the fork.

The main motivation behind this change is to avoid relying on a fork, which precludes publishing version 3.0.0 of the javy crates to crates.io, since git dependencies are not supported.

Another motivation, arguably more important than the one stated above, is making sure that we are always relying on the upstream implementation rather than in a fork containing patches maintained out-of-band.

Side-effects:

The fork enabled a slightly more performant execution in Wasm environments, when dealing with short-lived JS programs that don't require garbage collection, more explicitly, it introduced a patch that prevented QuickJS' __JS_FreeValueRT from running. Even though there are no known issues with this approach, __JS_FreeValueRT is an internal method and no guarantees are made regarding its stability, therefore it's preferred to avoid relying on it. Without this custom patch, I'm observing locally, on average a 2% increase in fuel usage outside of the 10% acceptable variance. Note that there are safe mechanisms in place to mitigate the impact of GC in the codebase, namely: (i) using ManuallyDrop where possible (ii) setting the GC threshold to -1 to reduce the possibility of any GC pauses. With that in mind, the 2% increase seems reasonable given all the benefits that we get from this change.

Alternatives considered:

  • Keeping the fork
    • Upgrades will be difficult
    • There's no easy way of publishing the javy crate
  • Post-processing the generated Wasm code and make JS_FreeValue and JS_FreeContext no-op.
    • This worked to a certain extent, however, in my opinion, the result is not worth the effort. This approach resulted in minimal performance improvements. Even though JS_FreeValue which is a method from the public API and is invoked by Rust drops will end up calling __JS_FreeValueRT, which is the most expensive of the free variants, not all __JS_FreeValueRT invocations stem from JS_FreeValue, some of them happen automatically from within the JS code.

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-core do not require updating CHANGELOG files.
  • I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • I've updated documentation including crate documentation if necessary.

This commit deprecrates the usage of the custom `rquickjs` fork and
pins the `rquickjs` version to 0.6.1, which is the current version in
the fork.

The main motivation behind this change is to avoid relying on a fork, which
precludes publishing version 3.0.0 of the `javy` crates to crates.io,
since git dependencies are not supported.

Another motivation, arguably more important than the one stated above,
is making sure that we are always relying on the upstream implementation
rather than in a fork containing patches maintained out-of-band.

Side-effects:

The fork enabled a slightly more performant execution in Wasm
environments, when dealing with short-lived JS programs that don't
require garbage collection, more explicitly, it introuced a patch that
prevented QuickJS' `__JS_FreeValueRT` from running. Even though there
are no known issues with this approach, `__JS_FreeValueRT` is an internal method and
no guarantees are made regarding its stability, therefore it's preferred
to avoid relying on it. Without this custom patch, I'm observing
locally, on average a 2% increase in fuel usage outside of the 10%
acceptable variance. Note that there are safe mechanisms in place to
mitigate the impact of GC in the codebase, namely: (i) using
`ManuallyDrop` where possible (ii) setting the GC threshold to `-1` to
reduce the possibility of any GC pauses. With that in mind, the 2% increase seems reasonable
given all the benefits that we get from this change.

Alternatives considered:

* Keeping the fork
  * Upgrades will be difficult
  * There's no easy way of publishing the `javy` crate
* Post-processing the generated Wasm code and make `JS_FreeValue` and
  `JS_FreeContext` no-op.
  * This worked to a certain extent, however, in my opinion, the result
    is not worth the effort. This approach resulted in minimal performance
    improvements. Even though `JS_FreeValue` which is a method from the
    public API and is invoked by Rust `drop`s will end up calling
    `__JS_FreeValueRT`, which is the most expensive of the `free`
    variants, not all `__JS_FreeValueRT` invocations stem from
    `JS_FreeValue`, some of them happen automatically from within the JS
    code.
@saulecabrera saulecabrera requested a review from jeffcharles June 27, 2024 19:44
@saulecabrera saulecabrera merged commit 93df556 into bytecodealliance:main Jun 28, 2024
14 checks passed
@saulecabrera saulecabrera deleted the remove-rquickjs-git-dep branch June 28, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants