Re-introduce call_arena #547
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
krichprollsch
approved these changes
Apr 22, 2025
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
force-pushed
the
jsruntime_arenas
branch
from
b83b23a to
3277d1b
Compare
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.
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
approved these changes
Apr 22, 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.
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_depthinteger 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.