Skip to content

Conversation

@karlseguin
Copy link
Collaborator

Because of callbacks, calls into Zig can be nested. Previously, the call_arena was reset after every call. When calls are nested, this doesn't work - the nested call resets the arena, which the caller might still need. A call_depth integer was added to the Executor. Each call starts by incrementing the call_depth and, on deinit, decrements the call_depth. The call_arena is only reset when the call_depth == 0. This could result in lower memory use.

Also promoted the call_arena and scope_arena to the Env. Practically speaking, nothing has changed, since they're still reset under the exact same conditions. However, when an executor ends and a new one is started, it can now reuse the retained_capacity of the previous arenas. This should result in fewer allocations.

@krichprollsch
Copy link
Member

@karlseguin can you rebase the branch please?

Because of callbacks, calls into Zig can be nested. Previously, the call_arena
was reset after _every_ call. When calls are nested, this doesn't work - the
nested call resets the arena, which the caller might still need. A `call_depth`
integer was added to the Executor. Each call starts by incrementing the
call_depth and, on deinit, decrements the call_depth. The call_arena is only
reset when the call_depth == 0. This could result in lower memory use.

Also promoted the call_arena and scope_arena to the Env. Practically speaking,
nothing has changed, since they're still reset under the exact same conditions.
However, when an executor ends and a new one is started, it can now reuse the
retained_capacity of the previous arenas. This should result in fewer
allocations.
@karlseguin
Copy link
Collaborator Author

@krichprollsch done

The call_arena is still re-added, and the call_depth is still used, but
the call_arena and scope_arenas are no longer part of the Env - they remain on
the Executor.

This is to accommodate upcoming changes where multiple executors will exist at
once. While the shared allocators _might_ have been safe in some cases, the
performance gains don't justify the risk of having 2 executors interacting in a
way where sharing the allocators would cause issues.
@karlseguin
Copy link
Collaborator Author

@krichprollsch This is ready. I reverted part of the change and keept the ArenaAllocator tied to the Executor. This should make having multiple Executors safe (it might be safe sharing it, but it would depend on how/if they interact, so this is just safer). PR still re-introduces the call_arena though, and the call_depth (which can be useful for other things), so still worth merging.

@krichprollsch krichprollsch merged commit 9727a9d into main Apr 22, 2025
12 checks passed
@krichprollsch krichprollsch deleted the jsruntime_arenas branch April 22, 2025 11:58
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants