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

Refactoring and bug fixes in the V8 API #13754

Merged
merged 40 commits into from
Sep 26, 2024
Merged

Refactoring and bug fixes in the V8 API #13754

merged 40 commits into from
Sep 26, 2024

Conversation

190n
Copy link
Contributor

@190n 190n commented Sep 5, 2024

What does this PR do?

Refactors and bugfixes in our V8 API implementation. So far:

  • Fix some places that did not handle oddballs (booleans or nullish JS values) properly (EscapableHandleScope, Global)
  • Support NewStringType::kInternalized
    • Previously the type was ignored. Now internalized strings are created with WTF::AtomString.
  • Remove or merge many functions that convert between V8 and JSC values
  • Make v8::Context be a Zig::GlobalObject instead of the same thing as v8::Isolate, which lets us access the global object with less indirection in functions that take a Context
    • side effect: there are now only 2 kinds of V8 handle, for cells and doubles, instead of 3. Context was the only place a void * handle was used.
  • Make v8::Isolate an actual class containing everything v8::Roots used to, and delete v8::Roots, instead of reinterpreting between Isolate and Roots
    • v8::Roots's only purpose was to hold values at the offsets from an Isolate pointer that V8 expects. Now we actually store those values in the Isolate like V8 does.
  • Test case and bug fix for EscapableHandleScope
  • Use m_ on C++ class field names for clarity
  • Move Bun-specific classes (i.e. classes that we use to implement the V8 API, but that don't actually exist in the real V8 API) into the namespace v8::shim.
    • All V8 classes that are kept in a Local (String, ObjectTemplate, etc.) now have zero fields, matching how V8's API works and making it less likely to crash by accessing a member while this is the invalid pointer that we get from V8. The cell pointer stored in the handle is either a built-in type (JSString, JSObject) or a type in v8::shim (v8::shim::InternalFieldObject, v8::shim::Function)
  • Move isosubspace declarations for V8 types next to the ones for other Bun-specific types
  • Support Function::GetName and Value::IsFunction
  • Make the this value received by native functions correct (it should be coerced to an object, or overridden by globalThis if null or undefined, like in JavaScript sloppy mode)
  • Add a ton of static assertions to check that we match important V8 implementation details

How did you verify your code works?

Most of the V8 API is covered by existing tests. A couple new tests were added.

190n added 19 commits August 29, 2024 17:43
The v8 namespace was confusingly polluted with both classes that exist
in the actual V8 API, as well as new classes to support Bun's specific
implementation of that API. This commit moves the latter into the
namespace v8::shim. It also makes a duplicate of classes with fields in
the v8::shim namespace. For those classes, the v8:: version has no
fields, no JSC superclass, and only implements the actual V8 API
functions, while the v8::shim:: version has the inheritance and
functions necessary to be allocated as a JSC object and implements
helper functions to support the V8 APIs.
@190n 190n changed the title Ben/v8 cleanup Refactoring and bug fixes in the V8 API Sep 5, 2024
@190n 190n marked this pull request as ready for review September 20, 2024 17:09
@Jarred-Sumner
Copy link
Collaborator

image

looks like some windows build issues?

@190n 190n merged commit 4e51f7d into main Sep 26, 2024
49 checks passed
@190n 190n deleted the ben/v8-cleanup branch September 26, 2024 23:54
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.

3 participants