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.

Sign up for GitHub

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

Jump to bottom

Address memory leak in JSON parse and stringify #835

Merged
merged 7 commits into from
Nov 21, 2024
Merged

Address memory leak in JSON parse and stringify #835

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
11a97e9
Address memory leak in JSON parse and stringify
jeffcharles Nov 19, 2024
1bbc149
Merge branch 'main' into jc.remove-memory-leak-in-json
jeffcharles Nov 19, 2024
f803afc
Only perform function lookups if we need to call the fallback
jeffcharles Nov 20, 2024
71bf2b3
Merge branch 'main' into jc.remove-memory-leak-in-json
jeffcharles Nov 20, 2024
3539d20
Replace fallback for stringify with rquickjs builtin
jeffcharles Nov 20, 2024
71cf16f
Use unstable key for default JSON.parse function
jeffcharles Nov 21, 2024
fe8ec83
Linting
jeffcharles Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions crates/javy/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

### Fixed

- Addressed memory leak when registering `JSON.parse` and `JSON.stringify`
functions.

## [3.0.2] - 2024-11-12

### Changed
Expand Down
17 changes: 12 additions & 5 deletions crates/javy/src/apis/json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,21 @@ impl Intrinsic for Json {

fn register<'js>(this: Ctx<'js>) -> Result<()> {
let global = this.globals();

const DEFAULT_PARSE_KEY: &str = "__javy_json_parse";
const DEFAULT_STRINGIFY_KEY: &str = "__javy_json_stringify";

let json: Object = global.get("JSON")?;
let default_parse: Function = json.get("parse")?;
global.set(DEFAULT_PARSE_KEY, default_parse)?;
let default_stringify: Function = json.get("stringify")?;
global.set(DEFAULT_STRINGIFY_KEY, default_stringify)?;

let parse = Function::new(
this.clone(),
MutFn::new(move |cx: Ctx<'js>, args: Rest<Value<'js>>| {
call_json_parse(hold!(cx.clone(), args), default_parse.clone())
.map_err(|e| to_js_error(cx, e))
MutFn::new(|cx: Ctx<'js>, args: Rest<Value<'js>>| {
let default_parse: Function = cx.globals().get(DEFAULT_PARSE_KEY)?;
call_json_parse(hold!(cx.clone(), args), default_parse).map_err(|e| to_js_error(cx, e))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, did you perform any benchmarking around this change? I don't think we have any fuel tests that cover this scenario, right? The reason why I suspect that this might affect performance is that every time any of these functions gets called, it'll result in a property access lookup, which is generally no the fastest operation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other consideration here is that this approach is going to pollute the global namespace, which might end up being used unofficially as an API (e.g., globalThis.__javy_json_stringify) which would be hard to deprecate down the line. Highly unlikely in my opinion but could happen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The readme test invokes JSON.parse and JSON.stringify. Fuel count on main is 253,787 and on this branch is 256,759 so ~1.17% increase given both method calls. I don't love that result and I'm open to other ideas for how to get rid of the memory leak.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is I could perform the fallback method look-up only if we can't call the method we're overriding it with (that is, if they pass more than one argument). Calls with more than one argument still be a bit more expensive than would otherwise be the case but at least the typical one argument case should be just as fast as it is now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the difference for a single argument is 254,153 on the branch versus 253,787 on main (~0.14% increase). I'm not sure why there's still an increase.

Copy link
Member

@saulecabrera saulecabrera Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative thought: have you considered introducing a cargo feature exclusively available for fuzzing? I suspect that might make your changes easier to integrate without making modifications that will be globally accessible for every user (e.g., potential performance impact and modifications to the global object).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the code we're fuzzing would be different code than the code users would use. Granted parts of the code being fuzzed would be the same but other parts would be different. And I suppose fuzzing some code is arguably better than no code being fuzzed. But if we do go this route, I would want to document in the project that enabling the JSON builtins introduces a known memory leak.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarification: when I meant fuzzing I meant to allow you to land this change with less friction and therefore help you detect the leak and how to fix it. From the description in your change it seems that there isn't full confidence that the JSON pieces are the cause of the leak?

Even if this isn't directly contributing to the out-of-memory errors during fuzzing, it would be very helpful for dump-leaks to not crash to aid in investigating if there are other memory leaks.

So the suggestion of the cargo feature is mostly to unblock any future investigations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because ideally I agree that we shouldn't modify the code one way or another to continuously fuzz.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there's a few things different things going on with the memory with a number of unknowns. I know the move of the QuickJS context as referenced by the Function into the MutFn introduces a memory leak. That can be verified that by turning on the dump-leaks Cargo feature on rquickjs or by removing the ManuallyDrop on the properties of the Javy Runtime and allowing the runtime to drop (an assertion will fire that the list of GC objects is not empty). What I don't know yet is if that memory leak only occurs once when a Javy runtime is created or if occurs every time JSON.parse and JSON.stringify are called. If it only happens once, then I don't think that would cause the memory growth we're seeing while fuzzing because we create a single runtime once. If it happens every time JSON.parse or JSON.stringify is called, then that would cause or exacerbate the memory growth we're seeing. But it happening once would still be a problem, independent of fuzzing, if someone wanted to drop the rquickjs runtime or rquickjs context for whatever reason. They can't drop either the rquickjs runtime or rquickjs context anyway with the APIs we currently expose but it would, for example, preclude being able to drop them in the future if we were to update the Javy crate to remove those ManualDrops and put it on the Javy Runtime instance in the javy-plugin-api.

}),
)?;

Expand All @@ -78,8 +84,9 @@ fn register<'js>(this: Ctx<'js>) -> Result<()> {

let stringify = Function::new(
this.clone(),
MutFn::new(move |cx: Ctx<'js>, args: Rest<Value<'js>>| {
call_json_stringify(hold!(cx.clone(), args), default_stringify.clone())
MutFn::new(|cx: Ctx<'js>, args: Rest<Value<'js>>| {
let default_stringify: Function = cx.globals().get(DEFAULT_STRINGIFY_KEY)?;
call_json_stringify(hold!(cx.clone(), args), default_stringify)
.map_err(|e| to_js_error(cx, e))
}),
)?;
Expand Down
Loading