From 8f562330c80b4c60d6056d5ec74efe74d9110271 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 20 Jul 2026 18:36:38 +0800 Subject: [PATCH] perf: Improve Runner tick signaling on macrotask-only wait Currently, Runner assumes that HttpClient.tick did something. But it's possible that HttpClient had nothing to do, and thus didn't poll. In that case, Runner would return .{.ok = 0} and Runner's caller would also certainly call Runner.tick again, resulting in a spin-loop. The reason Runner allows this to happen is because it can still have macrotasks to run. So now, when HttpClient.tick has done nothing, Runner will return its ms_to_next_task, rather than 0. On sites where all i/o is completed, and only macrotasks are waiting, this significantly reduces CPU usage. --- src/browser/Browser.zig | 8 ++++---- src/browser/Runner.zig | 17 +++++++++++------ src/browser/js/Env.zig | 13 +++++++++++-- src/browser/js/Scheduler.zig | 14 +++++++++----- src/cdp/CDP.zig | 2 +- src/network/HttpClient.zig | 28 +++++++++++++++++++++++++--- 6 files changed, 61 insertions(+), 21 deletions(-) diff --git a/src/browser/Browser.zig b/src/browser/Browser.zig index 54bb3e6f10..5841d46df4 100644 --- a/src/browser/Browser.zig +++ b/src/browser/Browser.zig @@ -215,12 +215,12 @@ pub fn waitForBackgroundTasks(self: *Browser) void { self.env.waitForBackgroundTasks(); } -pub fn msToNextMacrotask(self: *Browser) ?u64 { - return self.env.msToNextMacrotask(); +pub fn hasMacrotasks(self: *Browser) bool { + return self.env.hasMacrotasks(); } -pub fn msTo(self: *Browser) bool { - return self.env.hasBackgroundTasks(); +pub fn msToNextTask(self: *Browser) ?u64 { + return self.env.msToNextTask(); } pub fn runIdleTasks(self: *const Browser) void { diff --git a/src/browser/Runner.zig b/src/browser/Runner.zig index cf4e22d9cb..7ecc630618 100644 --- a/src/browser/Runner.zig +++ b/src/browser/Runner.zig @@ -171,7 +171,7 @@ fn _wait(self: *Runner, comptime is_cdp: bool, timeout_ms: u32, conditions: []Wa if (elapsed >= timeout_ms) { return .timeout; } - try self.http_client.tick(@min(timeout_ms - elapsed, 200)); + _ = try self.http_client.tick(@min(timeout_ms - elapsed, 200)); break :done_blk 0; }, }; @@ -225,9 +225,8 @@ fn _tick(self: *Runner, comptime is_cdp: bool, timeout_ms: u32, conditions: []Wa const total_http_activity = activity.http; const total_network_activity = activity.total(); - const ms_to_next_macrotask = browser.msToNextMacrotask(); const network_idle = activity.idle(); - const is_done = ms_to_next_macrotask == null and network_idle; + const is_done = browser.hasMacrotasks() == false and network_idle; // _we_ have nothing to run, but v8 is working on background tasks. We'll // wait for them. Don't do this for CDP, since new CDP messages can always @@ -296,13 +295,19 @@ fn _tick(self: *Runner, comptime is_cdp: bool, timeout_ms: u32, conditions: []Wa } if ((comptime is_cdp) or want_http_tick) { - var ms_to_wait = @min(timeout_ms, ms_to_next_macrotask orelse 200); + var ms_to_wait = @min(timeout_ms, browser.msToNextTask() orelse 200); if (browser.hasBackgroundTasks()) { // background work will queue more to do soon — don't block long // for a client message; loop back and run macrotasks instead. ms_to_wait = @min(ms_to_wait, 10); } - try http_client.tick(@intCast(ms_to_wait)); + const waited = try http_client.tick(@intCast(ms_to_wait)); + + // If the HttpClient didn't wait/poll then it has nothing to do, and we + // should tell our callerto wait until the next task is ready. + if ((comptime is_cdp) == false and waited == false) { + return .{ .ok = @intCast(ms_to_wait) }; + } return .{ .ok = 0 }; } @@ -310,7 +315,7 @@ fn _tick(self: *Runner, comptime is_cdp: bool, timeout_ms: u32, conditions: []Wa // But, if we have web socket connections, we still want to make progress on // them, so we tick with no delay. if (activity.ws_conns > 0 or activity.ws_events > 0) { - try http_client.tick(0); + _ = try http_client.tick(0); } return .done; diff --git a/src/browser/js/Env.zig b/src/browser/js/Env.zig index 7e13aa55f2..fa64dc0d47 100644 --- a/src/browser/js/Env.zig +++ b/src/browser/js/Env.zig @@ -461,10 +461,19 @@ pub fn runMacrotasks(self: *Env) !void { } } -pub fn msToNextMacrotask(self: *Env) ?u64 { +pub fn hasMacrotasks(self: *Env) bool { + for (self.contexts.items) |ctx| { + if (ctx.scheduler.high_priority.count() > 0) { + return true; + } + } + return false; +} + +pub fn msToNextTask(self: *Env) ?u64 { var next_task: u64 = std.math.maxInt(u64); for (self.contexts.items) |ctx| { - const candidate = ctx.scheduler.msToNextHigh() orelse continue; + const candidate = ctx.scheduler.msToNext() orelse continue; next_task = @min(candidate, next_task); } return if (next_task == std.math.maxInt(u64)) null else next_task; diff --git a/src/browser/js/Scheduler.zig b/src/browser/js/Scheduler.zig index 0e9d6ddbe7..42acb16800 100644 --- a/src/browser/js/Scheduler.zig +++ b/src/browser/js/Scheduler.zig @@ -99,13 +99,17 @@ pub fn hasReadyTasks(self: *Scheduler) bool { return queueHasReadyTask(&self.low_priority, now) or queueHasReadyTask(&self.high_priority, now); } -pub fn msToNextHigh(self: *Scheduler) ?u64 { - const task = self.high_priority.peek() orelse return null; +pub fn msToNext(self: *Scheduler) ?u64 { + var next: ?u64 = null; const now = milliTimestamp(.monotonic); - if (task.run_at <= now) { - return 0; + for ([_]*Queue{ &self.high_priority, &self.low_priority }) |queue| { + const task = queue.peek() orelse continue; + const ms = if (task.run_at <= now) 0 else task.run_at - now; + if (next == null or ms < next.?) { + next = ms; + } } - return @intCast(task.run_at - now); + return next; } fn runQueue(self: *Scheduler, queue: *Queue) !void { diff --git a/src/cdp/CDP.zig b/src/cdp/CDP.zig index 559396253f..7cf27b991c 100644 --- a/src/cdp/CDP.zig +++ b/src/cdp/CDP.zig @@ -271,7 +271,7 @@ pub fn tick(self: *CDP) !bool { // No active page yet (or a teardown is in flight). Fall // back to ticking the http client directly so CDP messages // still get dispatched. - self.browser.http_client.tick(wait_ms) catch |err| switch (err) { + _ = self.browser.http_client.tick(wait_ms) catch |err| switch (err) { error.ClientDisconnected => return false, else => { log.err(.app, "http tick", .{ .err = err }); diff --git a/src/network/HttpClient.zig b/src/network/HttpClient.zig index 183d8a6963..c6cab86cf9 100644 --- a/src/network/HttpClient.zig +++ b/src/network/HttpClient.zig @@ -562,7 +562,7 @@ pub fn newRequest(self: *Client, req: Request, owner: ?*Owner) anyerror!*Transfe return transfer; } -pub fn tick(self: *Client, timeout_ms: u32) !void { +pub fn tick(self: *Client, timeout_ms: u32) !bool { self.processGraveyard(); return self._tick(timeout_ms, .all); } @@ -571,18 +571,22 @@ pub fn tickSync(self: *Client, timeout_ms: u32) !void { if (self.hasPendingTeardown()) { return error.SyncWaitInterrupted; } - return self._tick(timeout_ms, .sync_wait); + _ = try self._tick(timeout_ms, .sync_wait); } fn hasPendingTeardown(self: *Client) bool { return self.inbox.contains(isSyncWaitInterrupt); } -pub fn _tick(self: *Client, timeout_ms: u32, mode: DrainMode) !void { +// Returns false iff the tick was a no-op. When false is returned, immediately +// calling this again will almost [instantly] return false again, potentially +// causing a spin. +pub fn _tick(self: *Client, timeout_ms: u32, mode: DrainMode) !bool { if (self.inbox.terminated) { return error.ClientDisconnected; } + var waited = true; const dispatched = self.dispatchCompleted(mode); try self.startPending(); @@ -603,6 +607,11 @@ pub fn _tick(self: *Client, timeout_ms: u32, mode: DrainMode) !void { // poll only waits, so we do the perform -> process dance again _ = try self.handles.perform(); _ = try self.processMessages(); + } else { + // There's nothing inflight. Polling will always wait until + // timeout_ms. Rather than do that, let's signal our caller (by + // returning false) and letting it decide what to do. + waited = false; } } else { // If we DID dispatch or process messages, we don't wan to wait / poll. @@ -617,6 +626,19 @@ pub fn _tick(self: *Client, timeout_ms: u32, mode: DrainMode) !void { // dispatch CDP commands try self.drainInbox(mode); + + if (comptime IS_DEBUG) { + if (waited == false) { + // we're about to tell our caller not to call us again without it + // doing some work (e.g. running tasks). Let's assert that we were + // right in doing that, else we'll likely introduce latency. + std.debug.assert(self.pending_queue.first == null); + std.debug.assert(self.dispatch_queue.first == null); + std.debug.assert(self.ws_dispatch_queue.first == null); + } + } + + return waited; } // Deliver completed response. This is the ONLY place user callbacks run,