Re-introduce call_arena

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.

Author: karlseguin

src/browser/dom/mutation_observer.zig

@@ -284,7 +284,7 @@ const EventHandler = struct {
         const muevt = parser.eventToMutationEvent(evt.?);
 
         // TODO get the allocator by another way?
-        const alloc = data.cbk.executor.call_arena.allocator();
+        const alloc = data.cbk.executor.scope_arena;
 
         if (std.mem.eql(u8, t, "DOMAttrModified")) {
             mrs.first = .{

src/runtime/js.zig

@@ -27,6 +27,9 @@ const ArenaAllocator = std.heap.ArenaAllocator;
 
 const log = std.log.scoped(.js);
 
+const CALL_ARENA_RETAIN = 1024 * 16;
+const SCOPE_ARENA_RETAIN = 1024 * 64;
+
 // Global, should only be initialized once.
 pub const Platform = struct {
     inner: v8.Platform,
@@ -177,6 +180,14 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
         // Send a lowMemoryNotification whenever we stop an executor
         gc_hints: bool,
 
+        // Used in debug mode to assert that we only have one executor at a time
+        has_executor: if (builtin.mode == .Debug) bool else void = if (builtin.mode == .Debug) false else {},
+
+        // See the Executor's call_arena and scope_arena allocators. Having these
+        // here allows the arena to be re-used by different executors.
+        call_arena: ArenaAllocator,
+        scope_arena: ArenaAllocator,
+
         const Self = @This();
 
         const State = S;
@@ -212,6 +223,8 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
                 .gc_hints = opts.gc_hints,
                 .global_scope = global_scope,
                 .prototype_lookup = undefined,
+                .call_arena = ArenaAllocator.init(allocator),
+                .scope_arena = ArenaAllocator.init(allocator),
                 .executor_pool = std.heap.MemoryPool(Executor).init(allocator),
             };
 
@@ -247,6 +260,8 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
         }
 
         pub fn deinit(self: *Self) void {
+            self.call_arena.deinit();
+            self.scope_arena.deinit();
             self.global_scope.deinit();
             self.isolate.exit();
             self.isolate.deinit();
@@ -260,6 +275,10 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
         }
 
         pub fn startExecutor(self: *Self, comptime Global: type, state: State, module_loader: anytype) !*Executor {
+            if (comptime builtin.mode == .Debug) {
+                std.debug.assert(self.has_executor == false);
+                self.has_executor = true;
+            }
             const isolate = self.isolate;
             const templates = &self.templates;
 
@@ -332,16 +351,14 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
                 context.setEmbedderData(1, data);
             }
 
-            const allocator = self.allocator;
-
             executor.* = .{
                 .state = state,
                 .context = context,
                 .isolate = isolate,
                 .templates = templates,
                 .handle_scope = handle_scope,
-                .call_arena = ArenaAllocator.init(allocator),
-                .scope_arena = ArenaAllocator.init(allocator),
+                .call_arena = self.call_arena.allocator(),
+                .scope_arena = self.scope_arena.allocator(),
                 .module_loader = .{
                     .ptr = @ptrCast(module_loader),
                     .func = @TypeOf(module_loader.*).fetchModuleSource,
@@ -375,6 +392,13 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
             if (self.gc_hints) {
                 self.isolate.lowMemoryNotification();
             }
+
+            if (comptime builtin.mode == .Debug) {
+                std.debug.assert(self.has_executor == true);
+                self.has_executor = false;
+            }
+            _ = self.call_arena.reset(.{ .retain_with_limit = CALL_ARENA_RETAIN });
+            _ = self.scope_arena.reset(.{ .retain_with_limit = SCOPE_ARENA_RETAIN });
         }
 
         // Give it a Zig struct, get back a v8.FunctionTemplate.
@@ -742,19 +766,25 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
             // a context, we can always get the Executor back.
             context: v8.Context,
 
+            // Because calls can be nested (i.e.a function calling a callback),
+            // we can only reset the call_arena when call_depth == 0. If we were
+            // to reset it within a callback, it would invalidate the data of
+            // the call which is calling the callback.
+            call_depth: usize = 0,
+
             // Arena whose lifetime is for a single getter/setter/function/etc.
             // Largely used to get strings out of V8, like a stack trace from
             // a TryCatch. The allocator will be owned by the Scope, but the
             // arena itself is owned by the Executor so that we can re-use it
             // from scope to scope.
-            call_arena: ArenaAllocator,
+            call_arena: Allocator,
 
             // Arena whose lifetime is for a single page load, aka a Scope. Where
             // the call_arena lives for a single function call, the scope_arena
             // lives for the lifetime of the entire page. The allocator will be
             // owned by the Scope, but the arena itself is owned by the Executor
             // so that we can re-use it from scope to scope.
-            scope_arena: ArenaAllocator,
+            scope_arena: Allocator,
 
             // When we need to load a resource (i.e. an external script), we call
             // this function to get the source. This is always a reference to the
@@ -777,13 +807,11 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
 
             // not public, must be destroyed via env.stopExecutor()
             fn deinit(self: *Executor) void {
-                if (self.scope) |*s| {
-                    s.deinit();
+                if (self.scope != null) {
+                    self.endScope();
                 }
                 self.context.exit();
                 self.handle_scope.deinit();
-                self.call_arena.deinit();
-                self.scope_arena.deinit();
             }
 
             // Executes the src
@@ -876,16 +904,16 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
                 v8.HandleScope.init(&handle_scope, self.isolate);
                 self.scope = Scope{
                     .handle_scope = handle_scope,
-                    .arena = self.scope_arena.allocator(),
-                    .call_arena = self.scope_arena.allocator(),
+                    .arena = self.scope_arena,
                 };
                 _ = try self._mapZigInstanceToJs(self.context.getGlobal(), global);
             }
 
             pub fn endScope(self: *Executor) void {
                 self.scope.?.deinit();
                 self.scope = null;
-                _ = self.scope_arena.reset(.{ .retain_with_limit = 1024 * 64 });
+                const scope_arena: *std.heap.ArenaAllocator = @ptrCast(@alignCast(self.scope_arena.ptr));
+                _ = scope_arena.reset(.{ .retain_with_limit = SCOPE_ARENA_RETAIN });
             }
 
             // Wrap a v8.Value, largely so that we can provide a convenient
@@ -1056,7 +1084,6 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
         // in executor, rather than having one for each of these.
         pub const Scope = struct {
             arena: Allocator,
-            call_arena: Allocator,
             handle_scope: v8.HandleScope,
             callbacks: std.ArrayListUnmanaged(v8.Persistent(v8.Function)) = .{},
             identity_map: std.AutoHashMapUnmanaged(usize, PersistentObject) = .{},
@@ -1113,7 +1140,7 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
 
                 self.call(args) catch |err| {
                     if (try_catch.hasCaught()) {
-                        const allocator = self.executor.scope.?.call_arena;
+                        const allocator = self.executor.call_arena;
                         result.stack = try_catch.stack(allocator) catch null;
                         result.exception = (try_catch.exception(allocator) catch @errorName(err)) orelse @errorName(err);
                     } else {
@@ -1144,7 +1171,7 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
             fn printFunc(self: *const @This()) !void {
                 const executor = self.executor;
                 const value = self.func.castToFunction().toValue();
-                const src = try valueToString(executor.call_arena.allocator(), value, executor.isolate, executor.context);
+                const src = try valueToString(executor.call_arena, value, executor.isolate, executor.context);
                 std.debug.print("{s}\n", .{src});
             }
         };
@@ -1168,7 +1195,7 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
             pub fn setIndex(self: JsObject, index: usize, value: anytype) !void {
                 const key = switch (index) {
                     inline 0...1000 => |i| std.fmt.comptimePrint("{d}", .{i}),
-                    else => try std.fmt.allocPrint(self.executor.scope_arena.allocator(), "{d}", .{index}),
+                    else => try std.fmt.allocPrint(self.executor.scope_arena, "{d}", .{index}),
                 };
                 return self.set(key, value);
             }
@@ -1421,7 +1448,7 @@ fn Caller(comptime E: type) type {
         context: v8.Context,
         isolate: v8.Isolate,
         executor: *E.Executor,
-        call_allocator: Allocator,
+        call_arena: Allocator,
 
         const Self = @This();
 
@@ -1433,17 +1460,34 @@ fn Caller(comptime E: type) type {
             const context = isolate.getCurrentContext();
             const executor: *E.Executor = @ptrFromInt(context.getEmbedderData(1).castTo(v8.BigInt).getUint64());
 
+            executor.call_depth += 1;
             return .{
                 .isolate = isolate,
                 .context = context,
                 .executor = executor,
-                .call_allocator = executor.scope.?.call_arena,
+                .call_arena = executor.call_arena,
             };
         }
 
         fn deinit(self: *Self) void {
-            _ = self;
-            // _ = self.executor.call_arena.reset(.{ .retain_with_limit = 4096 });
+            const call_depth = self.executor.call_depth - 1;
+            self.executor.call_depth = call_depth;
+
+            // Because of callbacks, calls can be nested. Because of this, we
+            // can't clear the call_arena after _every_ call. Imagine we have
+            //    arr.forEach((i) => { console.log(i); }
+            //
+            // First we call forEach. Inside of our forEach call,
+            // we call console.log. If we reset the call_arena after this call,
+            // it'll reset it for the `forEach` call after, which might still
+            // need the data.
+            //
+            // Therefore, we keep a call_depth, and only reset the call_arena
+            // when a top-level (call_depth == 0) function ends.
+            if (call_depth == 0) {
+                const call_arena: *std.heap.ArenaAllocator = @ptrCast(@alignCast(self.call_arena.ptr));
+                _ = call_arena.reset(.{ .retain_with_limit = CALL_ARENA_RETAIN });
+            }
         }
 
         fn constructor(self: *Self, comptime named_function: anytype, info: v8.FunctionCallbackInfo) !void {
@@ -1611,7 +1655,7 @@ fn Caller(comptime E: type) type {
         }
 
         fn nameToString(self: *Self, name: v8.Name) ![]const u8 {
-            return valueToString(self.call_allocator, .{ .handle = name.handle }, self.isolate, self.context);
+            return valueToString(self.call_arena, .{ .handle = name.handle }, self.isolate, self.context);
         }
 
         fn assertSelfReceiver(comptime named_function: anytype) void {
@@ -1660,7 +1704,7 @@ fn Caller(comptime E: type) type {
 
                     const Exception = comptime getCustomException(named_function.S) orelse break :blk null;
                     if (function_error_set == Exception or isErrorSetException(Exception, err)) {
-                        const custom_exception = Exception.init(self.call_allocator, err, named_function.js_name) catch |init_err| {
+                        const custom_exception = Exception.init(self.call_arena, err, named_function.js_name) catch |init_err| {
                             switch (init_err) {
                                 // if a custom exceptions' init wants to return a
                                 // different error, we need to think about how to
@@ -1793,7 +1837,7 @@ fn Caller(comptime E: type) type {
                         if (js_parameter_count == 0) {
                             @field(args, tupleFieldName(params_to_map.len + offset - 1)) = &.{};
                         } else {
-                            const arr = try self.call_allocator.alloc(last_parameter_type_info.pointer.child, js_parameter_count - params_to_map.len + 1);
+                            const arr = try self.call_arena.alloc(last_parameter_type_info.pointer.child, js_parameter_count - params_to_map.len + 1);
                             for (arr, last_js_parameter..) |*a, i| {
                                 const js_value = info.getArg(@as(u32, @intCast(i)));
                                 a.* = try self.jsValueToZig(named_function, slice_type, js_value);
@@ -1862,7 +1906,7 @@ fn Caller(comptime E: type) type {
                     },
                     .slice => {
                         if (ptr.child == u8) {
-                            return valueToString(self.call_allocator, js_value, self.isolate, self.context);
+                            return valueToString(self.call_arena, js_value, self.isolate, self.context);
                         }
 
                         // TODO: TypedArray
@@ -1887,7 +1931,7 @@ fn Caller(comptime E: type) type {
 
                         // Newer version of V8 appear to have an optimized way
                         // to do this (V8::Array has an iterate method on it)
-                        const arr = try self.call_allocator.alloc(ptr.child, js_arr.length());
+                        const arr = try self.call_arena.alloc(ptr.child, js_arr.length());
                         for (arr, 0..) |*a, i| {
                             a.* = try self.jsValueToZig(named_function, ptr.child, try js_obj.getAtIndex(context, @intCast(i)));
                         }