Skip to content

Commit

Permalink
Remove usage of the custom rquickjs fork (#675)
Browse files Browse the repository at this point in the history
* Remove usage of the custom `rquickjs` fork

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.

* Cargo vet

* Use `&Ctx<_>` in 262 tests

* Fix integration test
  • Loading branch information
saulecabrera authored Jun 28, 2024
1 parent ef05dbd commit 93df556
Show file tree
Hide file tree
Showing 9 changed files with 308 additions and 94 deletions.
14 changes: 10 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 8 additions & 8 deletions crates/cli/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ fn test_identity() {

let (output, _, fuel_consumed) = run_with_u8s(&mut runner, 42);
assert_eq!(42, output);
assert_fuel_consumed_within_threshold(43239, fuel_consumed);
assert_fuel_consumed_within_threshold(47_773, fuel_consumed);
}

#[test]
Expand All @@ -19,7 +19,7 @@ fn test_fib() {

let (output, _, fuel_consumed) = run_with_u8s(&mut runner, 5);
assert_eq!(8, output);
assert_fuel_consumed_within_threshold(59_822, fuel_consumed);
assert_fuel_consumed_within_threshold(66_007, fuel_consumed);
}

#[test]
Expand All @@ -28,7 +28,7 @@ fn test_recursive_fib() {

let (output, _, fuel_consumed) = run_with_u8s(&mut runner, 5);
assert_eq!(8, output);
assert_fuel_consumed_within_threshold(61_617, fuel_consumed);
assert_fuel_consumed_within_threshold(69_306, fuel_consumed);
}

#[test]
Expand All @@ -46,7 +46,7 @@ fn test_encoding() {

let (output, _, fuel_consumed) = run(&mut runner, "hello".as_bytes());
assert_eq!("el".as_bytes(), output);
assert_fuel_consumed_within_threshold(218_527, fuel_consumed);
assert_fuel_consumed_within_threshold(258_197, fuel_consumed);

let (output, _, _) = run(&mut runner, "invalid".as_bytes());
assert_eq!("true".as_bytes(), output);
Expand Down Expand Up @@ -76,7 +76,7 @@ fn test_readme_script() {

let (output, _, fuel_consumed) = run(&mut runner, r#"{ "n": 2, "bar": "baz" }"#.as_bytes());
assert_eq!(r#"{"foo":3,"newBar":"baz!"}"#.as_bytes(), output);
assert_fuel_consumed_within_threshold(235_476, fuel_consumed);
assert_fuel_consumed_within_threshold(270_919, fuel_consumed);
}

#[cfg(feature = "experimental_event_loop")]
Expand Down Expand Up @@ -106,7 +106,7 @@ fn test_exported_functions() {
let mut runner = Runner::new_with_exports("exported-fn.js", "exported-fn.wit", "exported-fn");
let (_, logs, fuel_consumed) = run_fn(&mut runner, "foo", &[]);
assert_eq!("Hello from top-level\nHello from foo\n", logs);
assert_fuel_consumed_within_threshold(72552, fuel_consumed);
assert_fuel_consumed_within_threshold(80023, fuel_consumed);
let (_, logs, _) = run_fn(&mut runner, "foo-bar", &[]);
assert_eq!("Hello from top-level\nHello from fooBar\n", logs);
}
Expand Down Expand Up @@ -180,7 +180,7 @@ fn test_exported_default_arrow_fn() {
);
let (_, logs, fuel_consumed) = run_fn(&mut runner, "default", &[]);
assert_eq!(logs, "42\n");
assert_fuel_consumed_within_threshold(67547, fuel_consumed);
assert_fuel_consumed_within_threshold(76706, fuel_consumed);
}

#[test]
Expand All @@ -192,7 +192,7 @@ fn test_exported_default_fn() {
);
let (_, logs, fuel_consumed) = run_fn(&mut runner, "default", &[]);
assert_eq!(logs, "42\n");
assert_fuel_consumed_within_threshold(67792, fuel_consumed);
assert_fuel_consumed_within_threshold(77909, fuel_consumed);
}

fn run_with_u8s(r: &mut Runner, stdin: u8) -> (u8, String, u64) {
Expand Down
2 changes: 1 addition & 1 deletion crates/javy-test-macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ fn gen_tests(

let test_contents = ::std::fs::read(&#path_str).expect("test file contents to be available");
let r: ::javy::quickjs::Result<()> = this.eval_with_options(test_contents, ::javy::quickjs::context::EvalOptions::default());
assert!(r.is_ok(), "{}", ::javy::val_to_string(this.clone(), this.catch()).unwrap());
assert!(r.is_ok(), "{}", ::javy::val_to_string(&this, this.catch()).unwrap());
});

}
Expand Down
4 changes: 3 additions & 1 deletion crates/javy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ categories = ["wasm"]

[dependencies]
anyhow = { workspace = true }
rquickjs = { git = "https://github.com/Shopify/rquickjs", branch = "improved-wasm-support", features = ["array-buffer", "bindgen", "no-free"] }
rquickjs = { version = "=0.6.1", features = ["array-buffer", "bindgen"] }
rquickjs-core = "=0.6.1"
rquickjs-sys = "=0.6.1"
serde = { version = "1.0", features = ["derive"] }
serde_json = { version = "1.0", optional = true }
serde-transcode = { version = "1.1", optional = true }
Expand Down
2 changes: 1 addition & 1 deletion crates/javy/src/apis/console/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ fn log<'js, T: Write>(args: Args<'js>, stream: &mut T) -> Result<Value<'js>> {
write!(stream, " ")?;
}

let str = val_to_string(ctx.clone(), arg)?;
let str = val_to_string(&ctx, arg)?;
write!(stream, "{str}")?;
}
writeln!(stream)?;
Expand Down
2 changes: 1 addition & 1 deletion crates/javy/src/apis/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ fn call_json_parse<'a>(args: Args<'a>, default: Function<'a>) -> Result<Value<'a
bail!(Exception::throw_type(&this, "Expected string primitive"));
}

let mut string = val_to_string(this.clone(), args[0].clone())?;
let mut string = val_to_string(&this, args[0].clone())?;
let bytes = unsafe { string.as_bytes_mut() };
json::parse(this.clone(), bytes).map_err(|original| {
if original.downcast_ref::<SError>().is_none() {
Expand Down
10 changes: 5 additions & 5 deletions crates/javy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ pub fn from_js_error(ctx: Ctx<'_>, e: JSError) -> Error {
if let Some(exception) = val.clone().into_exception() {
anyhow!("{exception}")
} else {
anyhow!(val_to_string(ctx, val).unwrap_or_else(|_| "Internal error".to_string()))
anyhow!(val_to_string(&ctx, val).unwrap_or_else(|_| "Internal error".to_string()))
}
} else {
Into::into(e)
Expand Down Expand Up @@ -202,21 +202,21 @@ pub fn to_string_lossy<'js>(cx: &Ctx<'js>, string: &JSString<'js>, error: JSErro
}

/// Retrieves the string representation of a JavaScript value.
pub fn val_to_string<'js>(this: Ctx<'js>, val: Value<'js>) -> Result<String> {
pub fn val_to_string<'js>(this: &Ctx<'js>, val: Value<'js>) -> Result<String> {
if let Some(symbol) = val.as_symbol() {
if let Some(description) = symbol.description()?.into_string() {
let description = description
.to_string()
.unwrap_or_else(|e| to_string_lossy(&this, &description, e));
.unwrap_or_else(|e| to_string_lossy(this, &description, e));
Ok(format!("Symbol({description})"))
} else {
Ok("Symbol()".into())
}
} else {
let stringified = <convert::Coerced<JSString>>::from_js(&this, val).map(|string| {
let stringified = <convert::Coerced<JSString>>::from_js(this, val).map(|string| {
string
.to_string()
.unwrap_or_else(|e| to_string_lossy(&this, &string.0, e))
.unwrap_or_else(|e| to_string_lossy(this, &string.0, e))
})?;
Ok(stringified)
}
Expand Down
80 changes: 16 additions & 64 deletions supply-chain/config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -358,22 +358,6 @@ criteria = "safe-to-deploy"
version = "0.1.60"
criteria = "safe-to-deploy"

[[exemptions.icu_collections]]
version = "1.5.0"
criteria = "safe-to-deploy"

[[exemptions.icu_locid]]
version = "1.5.0"
criteria = "safe-to-deploy"

[[exemptions.icu_locid_transform]]
version = "1.5.0"
criteria = "safe-to-deploy"

[[exemptions.icu_locid_transform_data]]
version = "1.5.0"
criteria = "safe-to-deploy"

[[exemptions.icu_normalizer]]
version = "1.5.0"
criteria = "safe-to-deploy"
Expand All @@ -382,22 +366,6 @@ criteria = "safe-to-deploy"
version = "1.5.0"
criteria = "safe-to-deploy"

[[exemptions.icu_properties]]
version = "1.5.0"
criteria = "safe-to-deploy"

[[exemptions.icu_properties_data]]
version = "1.5.0"
criteria = "safe-to-deploy"

[[exemptions.icu_provider]]
version = "1.5.0"
criteria = "safe-to-deploy"

[[exemptions.icu_provider_macros]]
version = "1.5.0"
criteria = "safe-to-deploy"

[[exemptions.idna]]
version = "1.0.0"
criteria = "safe-to-deploy"
Expand Down Expand Up @@ -474,10 +442,6 @@ criteria = "safe-to-deploy"
version = "0.1.3"
criteria = "safe-to-deploy"

[[exemptions.litemap]]
version = "0.7.3"
criteria = "safe-to-deploy"

[[exemptions.log]]
version = "0.4.21"
criteria = "safe-to-deploy"
Expand Down Expand Up @@ -638,6 +602,22 @@ criteria = "safe-to-deploy"
version = "1.3.0"
criteria = "safe-to-deploy"

[[exemptions.rquickjs]]
version = "0.6.1"
criteria = "safe-to-deploy"

[[exemptions.rquickjs-core]]
version = "0.6.1"
criteria = "safe-to-deploy"

[[exemptions.rquickjs-macro]]
version = "0.6.1"
criteria = "safe-to-deploy"

[[exemptions.rquickjs-sys]]
version = "0.6.1"
criteria = "safe-to-deploy"

[[exemptions.rustc-demangle]]
version = "0.1.24"
criteria = "safe-to-deploy"
Expand Down Expand Up @@ -794,10 +774,6 @@ criteria = "safe-to-deploy"
version = "3.10.1"
criteria = "safe-to-deploy"

[[exemptions.tinystr]]
version = "0.7.6"
criteria = "safe-to-deploy"

[[exemptions.tower]]
version = "0.4.13"
criteria = "safe-to-deploy"
Expand Down Expand Up @@ -934,22 +910,10 @@ criteria = "safe-to-deploy"
version = "1.0.0"
criteria = "safe-to-deploy"

[[exemptions.writeable]]
version = "0.5.5"
criteria = "safe-to-deploy"

[[exemptions.wyz]]
version = "0.5.1"
criteria = "safe-to-deploy"

[[exemptions.yoke]]
version = "0.7.4"
criteria = "safe-to-deploy"

[[exemptions.yoke-derive]]
version = "0.7.4"
criteria = "safe-to-deploy"

[[exemptions.zerocopy]]
version = "0.7.34"
criteria = "safe-to-deploy"
Expand All @@ -958,22 +922,10 @@ criteria = "safe-to-deploy"
version = "0.7.34"
criteria = "safe-to-deploy"

[[exemptions.zerofrom]]
version = "0.1.4"
criteria = "safe-to-deploy"

[[exemptions.zerofrom-derive]]
version = "0.1.4"
criteria = "safe-to-deploy"

[[exemptions.zerovec]]
version = "0.10.2"
criteria = "safe-to-deploy"

[[exemptions.zerovec-derive]]
version = "0.10.2"
criteria = "safe-to-deploy"

[[exemptions.zstd]]
version = "0.13.1"
criteria = "safe-to-deploy"
Expand Down
Loading

0 comments on commit 93df556

Please sign in to comment.