replace zig-js-runtime #506
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
karlseguin
force-pushed
the
jsruntime
branch
2 times, most recently
from
18f9d9f to
b8c7c95
Compare
karlseguin
force-pushed
the
jsruntime
branch
2 times, most recently
from
6a254e4 to
089a641
Compare
Member
|
FYI I get a non-blocking warning when running |
Collaborator
Author
|
I saw this in the original too. |
Member
|
I tried the puppeteer |
Member
|
We had a benchmark test to track zig-js-runtime perf (https://perf.lightpanda.io/benchmark/jsruntime) But it coud be useful to track the performance someway to detect regressions. |
Member
|
This PR is huge, but well explained. The comments in |
Remove Env from caller, and don't store Env in isolate. We don't need it, we can execute all runtime code from the Executor (which we store a pointer to in the v8.Context)
- Fix passing nothing into variadic (i.e. slice) parameter - Optimize @sizeof(T) == 0 types by avoiding uncessary allocations (something zig-js-runtime is doing)
Add dummy --json stats output to tests Comment typos fixed Add make get-v8, build-v8 and build-v8-dev make targets
Remove index and named setters, since they aren't working, and they aren't currently needed.
index_get seems to be ~1000x slower than setting the value directly on the v8.Object. There's a lot of information on "v8 fast properties", and values set directly on objects seem to be heavily optimized. Still, I can't imagine indexed properties are always _that_ slow, so I must be doing something wrong. Still, for now, this brings back the original functionality / behavior / perf. Introduce the ability for Zig functions to take a Env.JsObject parameter. While this isn't currently being used, it aligns with bringing back the postAttach / JSObject functionality in main. Moved function *State to the end of the function list (making it consistent with getters and setters). The optional Env.JsObject parameter comes after the optional state. Removed some uncessary arena deinits from a few webapis.
Having a js source name is useful to detect where the error comes from. Using `null` generates messages with `<anonymous>` source name. eg. `ReferenceError: report is not defined\n at <anonymous>:1:1` vs. `ReferenceError: report is not defined\n at teststatus:1:1`
test: re-introduce js source name
The call arena doesn't consider nested calls (like, from callbacks). Currently when a "call" ends, the arena is cleared. But in a callback, if we do that, the memory for the containing code is no longer valid, even though it's still executing. For now, use the existing scope_arena, instead of the call_arena. In the future we need to track the call-depth, and only reset the call_arena when we're done with a top-level statement. Also: -Properly handle callback errors -Increase wpt file size -Merge latest loop.zig from zig-js-runtime.
krichprollsch
approved these changes
Apr 17, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-v8followed by
zig build build-v8first. It might be better to test this branchin 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_hintscommand line argument. Note that without the--gc_hintsmemory usagedoes level off at some point, and it's faster. With
--gc_hints, the memoryconsumption should be lower than before and it might still be a bit faster.
Overview
This PR aims to replace
zig-js-runtimewith the main goal of:by the http client rewrite).
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-forkandtigerbeetle-ioare now Zig packages. Both projects werechanged slightly to be proper Zig libraries. They both have a
jsruntimebranchwhich this branch references.
netsurf.zigandmimalloc.zigare no longer separate modules and can be found in:src/browser/netsurf.zigandsrc/browser/mimalloc.zig.zig build unittesthas been removed.zig build testis now more idiomatic:the root source file is src/main.zig and tests are automatically discovered.
zig build testis, 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:
Browser/Session/Page, the main integration points with the js runtime,have changed slightly.
UserContexthas been renamed toSessionStateand, while the same in spirit,is also quite different.
LoopandAllocatorare no longer "internal" or special types. These are nowaccessed through the SessionState.
Variadicno longer exists, they map directly to plain Zig slicespostAttachhas been replaced withindexed_getandnamed_getwhichcapture live changes.
mem_guarantiedis no longer needed and has been removed where declared.get-v8which gets the tools to build v8as well as the v8 source, and
build-v8which builds v8. All of this is placedin a
v8folder at the root of the project. Also, on mac, we always build a releasebuild (since the dev build segfaults).
js runtime
The zig-js-runtime project has been replaced with two files
src/runtime/loop.zigandsrc/runtime/js.zig.The loop is unchanged except that
cancelon 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_hintscommand line argument, we callisolate.lowMemoryNotification()wheneach 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
reflectstage nor an internal type representation. Themain 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 isStategeneric. From the rest of Browser's point of view,it is now called
SessionState.The
SessionStatebelongs to theSessionand is referenced by theExecutor.Rather than calling
setUserContext, thesession.stateobject ismutated directly.
Most importantly, the js runtime no longer has the concept of special/internal
types. Thus,
*LoopandAllocatorhave been added to the SessionState.For example, the
XMLHttpRequestconstructor was:And is now:
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.