Address memory leak in JSON parse and stringify

[jeffcharles]

Description of the change

Put default function implementations of JSON.parse and JSON.stringify into global object instead of moving a clone of them into function implementations.

Why am I making this change?

Enabling the dump-leaks feature on rquickjs and dropping the QuickJS runtime and context had the following output:

Object leaks:
       ADDRESS REFS SHRF          PROTO      CLASS PROPS
      0x130230    2 [js_context]
      0x134400    1   0*       0x130640   Function { length: 2, name: 3'parse' }
      0x134460    1   0*       0x130640   Function { length: 3, name: 2'stringify' }
Assertion failed: list_empty(&rt->gc_obj_list) (/Users/jeffcharles/src/github.com/Shopify/javy/target/wasm32-wasip1/release/build/rquickjs-sys-9b428111e744954d/out/quickjs.c: JS_FreeRuntime: 2002)
Error: the `initialize_runtime` function trapped

Fuzzing is failing at the moment with out of memory errors. 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.

Checklist

• I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-plugin 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.

[saulecabrera]

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.

[saulecabrera]

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.

[jeffcharles]

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.

[jeffcharles]

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.

[jeffcharles]

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.

[saulecabrera]

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).

[jeffcharles]

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.

[saulecabrera]

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.

[saulecabrera]

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

[jeffcharles]

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.

[jeffcharles]

Current performance difference for the README test is 253,787 on main and 254,592 on this branch (~0.31% increase). I'm not sure why there's an increase. If anything, I expected there should've been a decrease from avoiding cloning the default parse and stringify functions.