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

replace zig-js-runtime #506

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

replace zig-js-runtime #506

wants to merge 2 commits into from

Conversation

karlseguin
Copy link
Collaborator

@karlseguin karlseguin commented Apr 6, 2025

Big PR description, but 2 important things to highlight:

1 -
The build process has changed a lot. You'll need to run zig build get-v8
followed by zig build build-v8 first. It might be better to test this branch
in a separate / clean folder (it'll leave a large v8 folder in the root of the
project).

2 -
If you run this and notice that memory usage is high, try running it with the new
--gc_hints command line argument. Note that without the --gc_hints memory usage
does level off at some point, and it's faster. With --gc_hints, the memory
consumption should be lower than before and it might still be a bit faster.

Overview

This PR aims to replace zig-js-runtime with the main goal of:

  1. Removing the global objects (Types and UserContext). (IO was largely removed
    by the http client rewrite).
  2. Reworking problematic and unsafe proto implementation (i.e.
    No memory layout guarantee with zig structs zig-js-runtime#205)

The intention wasn't to improve performance or memory usage, but, at least on
my tests, both have been improved.

The changes can be broken down into two parts, changes to Browser and reworking
of zig-js-runtime.

Browser Changes

Most of the changes to browser were made to accommodate the new js runtime.
However, other changes were also made:

  • All browser-related code was moved under the browser folder. For example,
    src/css/ -> src/browser/css/

  • zig-v8-fork and tigerbeetle-io are now Zig packages. Both projects were
    changed slightly to be proper Zig libraries. They both have a jsruntime branch
    which this branch references.

  • netsurf.zig and mimalloc.zig are no longer separate modules and can be found in: src/browser/netsurf.zig and src/browser/mimalloc.zig.

  • zig build unittest has been removed. zig build test is now more idiomatic:
    the root source file is src/main.zig and tests are automatically discovered.
    zig build test is, as before, slow and something to be looked into.

  • All existing web API tests are passing, but the test runner has changed a little
    so every single line of test has been superficially touched. Also,
    getter can't return a Callback zig-js-runtime#200 is now fixed, so
    the XHR test no long has commented out tests cases.

  • The rest of the changes exist specifically to accommodate the new jsruntime,
    which will be covered next, but briefly:

    1. Browser / Session / Page, the main integration points with the js runtime,
      have changed slightly.
    2. UserContext has been renamed to SessionState and, while the same in spirit,
      is also quite different.
    3. Loop and Allocator are no longer "internal" or special types. These are now
      accessed through the SessionState.
    4. Callback API changed a little
    5. Variadic no longer exists, they map directly to plain Zig slices
    6. postAttach has been replaced with indexed_get and named_get which
      capture live changes.
    7. mem_guarantied is no longer needed and has been removed where declared.
    8. 2 new build targets: get-v8 which gets the tools to build v8
      as well as the v8 source, and build-v8 which builds v8. All of this is placed
      in a v8 folder at the root of the project. Also, on mac, we always build a release
      build (since the dev build segfaults).

js runtime

The zig-js-runtime project has been replaced with two files
src/runtime/loop.zig and src/runtime/js.zig.

The loop is unchanged except that cancel on Mac no longer allocates
(and leaks) memory (it's a complete no-op, versus the current partial no-op).

Architecture

The js runtime architecture matches the Browser structure:

Browser -> js.Env -> v8.Isolate
Session -> js.Executor -> v8.Context
Page -> js.Scope -> v8.HandleScope

As you can see, the v8.Isolate has been elevated to the Browser and is re-used
across multiple sessions. To make this possible, most (maybe all??) leaks have
been fixed. However, v8.Context are garbage collected by v8 at its own discretion.
Thus, this new approach, while faster, is likely to use more memory. With the new
--gc_hints command line argument, we call isolate.lowMemoryNotification() when
each executor ends - encouraging v8 to garbage collect the just-released
context. Note that this is still just a hint. I think it's safe to say that this
approach gives more control to the v8 GC, making memory usage less predictable.

Code Generation

The v8 bindings are created in a single-pass, directly from Zig's type system.
There is no longer a reflect stage nor an internal type representation. The
main advantage is that you don't need to learn two type systems.

Generics

The global types have been replaced by making most js.* types (i.e. js.Env)
generics. I believe this significantly simplifies our build process, thus the
ability to have the tests' root source file be src/main.zig.

The only downside that I've seen is that you will get an awful type description
in an error message involving the js.Env(T) or any of its child types.

UserContext / SessionState / State

UserContext has been reworked. From the js runtime's point of view, i.e.
js.Env(T), this is State generic. From the rest of Browser's point of view,
it is now called SessionState.

The SessionState belongs to the Session and is referenced by the Executor.
Rather than calling setUserContext, the session.state object is
mutated directly.

Most importantly, the js runtime no longer has the concept of special/internal
types. Thus, *Loop and Allocator have been added to the SessionState.
For example, the XMLHttpRequest constructor was:

pub fn constructor(alloc: std.mem.Allocator, userctx: UserContext) !XMLHttpRequest {
  ...
}

And is now:

pub fn constructor(session_state: *SessionState) !XMLHttpRequest {
  const arena = session_state.arena;
  ...
}

Compatibility with other engines

Without knowing anything about other JS engines, I feel that some of the changes
could make integrating a different JS engine harder and some of the changes
could make it simpler. I think this new approaches exposes a few less types and
presents more of a self-contained, but far from perfect, interface.

Personally, I think it would be great if the abstraction could be built under this layer, so that instead of having a v8.Context, v8.Value and v8.Object, we have a js.Context, js.Value and js.Object, so that binding code can be re-used, but that might be a stretch.

@krichprollsch
Copy link
Member

FYI I get a non-blocking warning when running build-v8

$ zig build build-v8
WARNING at the command-line "--args":1:76: Build argument has no effect.
target_os="linux" target_cpu="x64" is_debug=true symbol_level=1 cxx_use_ld="lld"
                                                                           ^----
The variable "cxx_use_ld" was set as a build argument
but never appeared in a declare_args() block in any buildfile.

To view all possible args, run "gn args --list <out_dir>"

The build continued as if that argument was unspecified.

Done. Made 183 targets from 100 files in 99ms
ninja: Entering directory `v8/build/x86_64-linux/debug/ninja'
[882/1972] CXX obj/torque_generated_definitions/feedback-vector-tq.o

@karlseguin
Copy link
Collaborator Author

I saw this in the original too.

try gn_args.append("cxx_use_ld="lld"");

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.

2 participants