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

[STAL-1960] Fix performance issues introduced by creating new v8 Contexts #416

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

jasonforal
Copy link
Collaborator

@jasonforal jasonforal commented Jun 27, 2024

What problem are you trying to solve?

TL;DR fixing some performance regressions I introduced.


With #398, we implemented scoped_execute, which runs a script within a v8::Context that is a "blank slate" across executions, so no script can mutate the state in a way that persists to another execution.

I did this by creating a new v8::Context for every execution, with the justification that the overall performance hit was acceptable.

However, I made some mistakes in that profiling, and upon re-visiting, it looks like the impact is significant (~15% performance penalty).

Before (122 seconds

v8 spends a lot of time creating the new contexts (~11% of observed frames).
new-context

What is your solution?

We need two things:

  1. The global state should not be mutable from JavaScript.
  2. The global state should be mutable from Rust.

Technical Details

We now re-use the same v8::Context across executions. To achieve an equivalent level of encapsulation as creating a new context, we employ two strategies:

Freeze the global object and set its prototype
In JavaScript, calling Object.freeze(/* obj */) on an object prevents new properties from being added, existing properties from being modified in any way, and the object's prototype from being changed (reference). We freeze the v8 context's default global object to satisfy #1. However, to satisfy #2, we can't set our bridge variables on the global object directly (because a frozen object cannot be mutated by either JS or Rust). Instead, we set our own persistent object as the prototype of the v8::Context's global object. When a script accesses, e.g. a ddsa bridge variable name, it will walk the prototype chain and resolve to a value on our persistent object, allowing us to achieve #2.

Wrap rule code in an anonymous function
An anonymous function provides encapsulation by creating a separate scope for all code within it.

For example, a function declaration is effectively an assignment to the scope's most immediate global object:

function greet() { /* ... */ }
// Equivalent to:
//
// globalThis.greet = () => { /* ... */ };

Because we've frozen globalThis, a standard function declaration won't work. Instead, we have to enter a different scope with an anonymous function:

(() => {
  function greet() { /* ... */ }
  // Equivalent to:
  //
  // const greet = () => { /* ... */ };
})();

This prevents local variable declarations from applying to/mutating the v8 context we're re-using.

"use strict";
We set the script to run in strict mode. This is desirable because it makes code that attempts to write to an unwritable object (e.g. the frozen globalThis) throw an error (instead of silently fail).

After (107 seconds

reused-context

Alternatives considered

What the reviewer should know

  • The stella library in production also re-uses a v8 context, but this PR takes the encapsulation further.
  • Disregard 436349e, which was necessary to keep stella working. This will be after switching over to ddsa.

@jasonforal jasonforal requested a review from juli1 June 27, 2024 19:15
@jasonforal jasonforal merged commit e0f4f76 into main Jun 28, 2024
62 checks passed
@jasonforal jasonforal deleted the jf/STAL-1960_context branch June 28, 2024 21:42
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.

None yet

2 participants