Start downloading all synchronous imports ASAP

This changes how non-async module loading works. In general, module loading
is triggered by a v8 callback. We ask it to process a module (a <script type=
module>) and then for every module that it depends on, we get a callback. This
callback expects the nested v8.Module instance, so we need to load it then and
there (as opposed to dynamic imports, where we only have to return a promise).

Previously, we solved this by issuing a blocking HTTP get in each callback. The
HTTP loop was able to continuing downloading already-queued resources, but if
a module depended on 20 nested modules, we'd issue 20 blocking gets one after
the other.

Once a module is compiled, we can ask v8 for a list of its dependent module. We
can them immediately start to download all of those modules. We then evaluate
the original module, which will trigger our callback. At this point, we still
need to block and wait for the response, but we've already started the download
and it's much faster. Sure, for the first module, we might need to wait the same
amount of time, but for the other 19, chances are by the time the callback
executes, we already have it downloaded and ready.

Author: karlseguin

src/browser/ScriptManager.zig

@@ -67,8 +67,18 @@ client: *Http.Client,
 allocator: Allocator,
 buffer_pool: BufferPool,
 script_pool: std.heap.MemoryPool(PendingScript),
+sync_module_pool: std.heap.MemoryPool(SyncModule),
 async_module_pool: std.heap.MemoryPool(AsyncModule),
 
+// We can download multiple sync modules in parallel, but we want to process
+// then in order. We can't use an OrderList, like the other script types,
+// because the order we load them might not be the order we want to process
+// them in (I'm not sure this is true, but as far as I can tell, v8 doesn't
+// make any guarantees about the list of sub-module dependencies it gives us
+// So this is more like a cache. When a SyncModule is complete, it's put here
+// and can be requested as needed.
+sync_modules: std.StringHashMapUnmanaged(*SyncModule),
+
 const OrderList = std.DoublyLinkedList;
 
 pub fn init(browser: *Browser, page: *Page) ScriptManager {
@@ -80,24 +90,42 @@ pub fn init(browser: *Browser, page: *Page) ScriptManager {
         .scripts = .{},
         .deferreds = .{},
         .asyncs_ready = .{},
+        .sync_modules = .empty,
         .is_evaluating = false,
         .allocator = allocator,
         .client = browser.http_client,
         .static_scripts_done = false,
         .buffer_pool = BufferPool.init(allocator, 5),
         .script_pool = std.heap.MemoryPool(PendingScript).init(allocator),
+        .sync_module_pool = std.heap.MemoryPool(SyncModule).init(allocator),
         .async_module_pool = std.heap.MemoryPool(AsyncModule).init(allocator),
     };
 }
 
 pub fn deinit(self: *ScriptManager) void {
     self.reset();
+    var it = self.sync_modules.valueIterator();
+    while (it.next()) |value_ptr| {
+        value_ptr.*.buffer.deinit(self.allocator);
+        self.sync_module_pool.destroy(value_ptr.*);
+    }
+
     self.buffer_pool.deinit();
     self.script_pool.deinit();
+    self.sync_module_pool.deinit();
     self.async_module_pool.deinit();
+
+    self.sync_modules.deinit(self.allocator);
 }
 
 pub fn reset(self: *ScriptManager) void {
+    var it = self.sync_modules.valueIterator();
+    while (it.next()) |value_ptr| {
+        value_ptr.*.buffer.deinit(self.allocator);
+        self.sync_module_pool.destroy(value_ptr.*);
+    }
+    self.sync_modules.clearRetainingCapacity();
+
     self.clearList(&self.asyncs);
     self.clearList(&self.scripts);
     self.clearList(&self.deferreds);
@@ -259,49 +287,70 @@ pub fn addFromElement(self: *ScriptManager, element: *parser.Element) !void {
 // Unlike external modules which can only ever be executed after releasing an
 // http handle, these are executed without there necessarily being a free handle.
 // Thus, Http/Client.zig maintains a dedicated handle for these calls.
-pub fn blockingGet(self: *ScriptManager, url: [:0]const u8) !GetResult {
-    std.debug.assert(self.is_blocking == false);
-
-    self.is_blocking = true;
-    defer {
-        self.is_blocking = false;
-
-        // we blocked evaluation while loading this script, there could be
-        // scripts ready to process.
-        self.evaluate();
+pub fn getModule(self: *ScriptManager, url: [:0]const u8) !void {
+    const gop = try self.sync_modules.getOrPut(self.allocator, url);
+    if (gop.found_existing) {
+        // already requested
+        return;
     }
+    errdefer _ = self.sync_modules.remove(url);
 
-    var blocking = Blocking{
-        .allocator = self.allocator,
-        .buffer_pool = &self.buffer_pool,
-    };
+    const sync = try self.sync_module_pool.create();
+    errdefer self.sync_module_pool.destroy(sync);
+
+    sync.* = .{ .manager = self };
+    gop.value_ptr.* = sync;
 
     var headers = try self.client.newHeaders();
     try self.page.requestCookie(.{}).headersForRequest(self.page.arena, url, &headers);
 
-    var client = self.client;
-    try client.blockingRequest(.{
+    try self.client.request(.{
         .url = url,
+        .ctx = sync,
         .method = .GET,
         .headers = headers,
         .cookie_jar = self.page.cookie_jar,
-        .ctx = &blocking,
         .resource_type = .script,
-        .start_callback = if (log.enabled(.http, .debug)) Blocking.startCallback else null,
-        .header_callback = Blocking.headerCallback,
-        .data_callback = Blocking.dataCallback,
-        .done_callback = Blocking.doneCallback,
-        .error_callback = Blocking.errorCallback,
+        .start_callback = if (log.enabled(.http, .debug)) SyncModule.startCallback else null,
+        .header_callback = SyncModule.headerCallback,
+        .data_callback = SyncModule.dataCallback,
+        .done_callback = SyncModule.doneCallback,
+        .error_callback = SyncModule.errorCallback,
     });
+}
+
+pub fn waitForModule(self: *ScriptManager, url: [:0]const u8) !GetResult {
+    std.debug.assert(self.is_blocking == false);
+    self.is_blocking = true;
+    defer self.is_blocking = false;
+
+    // Normally it's dangerous to hold on to map pointers. But here, the map
+    // can't change. It's possible that by calling `tick`, other entries within
+    // the map will have their value change, but the map itself is immutable
+    // during this tick.
+    const entry = self.sync_modules.getEntry(url) orelse {
+        return error.UnknownModule;
+    };
+    const sync = entry.value_ptr.*;
 
-    // rely on http's timeout settings to avoid an endless/long loop.
+    var client = self.client;
     while (true) {
-        _ = try client.tick(200);
-        switch (blocking.state) {
-            .running => {},
-            .done => |result| return result,
+        switch (sync.state) {
+            .loading => {},
+            .done => {
+                // Our caller has its own higher level cache (caching the
+                // actual compiled module). There's no reason for us to keep this
+                defer self.sync_module_pool.destroy(sync);
+                defer self.sync_modules.removeByPtr(entry.key_ptr);
+                return .{
+                    .buffer = sync.buffer,
+                    .buffer_pool = &self.buffer_pool,
+                };
+            },
             .err => |err| return err,
         }
+        // rely on http's timeout settings to avoid an endless/long loop.
+        _ = try client.tick(200);
     }
 }
 
@@ -332,7 +381,6 @@ pub fn getAsyncModule(self: *ScriptManager, url: [:0]const u8, cb: AsyncModule.C
         .error_callback = AsyncModule.errorCallback,
     });
 }
-
 pub fn staticScriptsDone(self: *ScriptManager) void {
     std.debug.assert(self.static_scripts_done == false);
     self.static_scripts_done = true;
@@ -782,16 +830,15 @@ const BufferPool = struct {
     }
 };
 
-const Blocking = struct {
-    allocator: Allocator,
-    buffer_pool: *BufferPool,
-    state: State = .{ .running = {} },
+const SyncModule = struct {
+    manager: *ScriptManager,
     buffer: std.ArrayListUnmanaged(u8) = .{},
+    state: State = .loading,
 
     const State = union(enum) {
-        running: void,
+        done,
+        loading,
         err: anyerror,
-        done: GetResult,
     };
 
     fn startCallback(transfer: *Http.Transfer) !void {
@@ -807,12 +854,13 @@ const Blocking = struct {
             .content_type = header.contentType(),
         });
 
+        var self: *SyncModule = @ptrCast(@alignCast(transfer.ctx));
         if (header.status != 200) {
+            self.finished(.{ .err = error.InvalidStatusCode });
             return error.InvalidStatusCode;
         }
 
-        var self: *Blocking = @ptrCast(@alignCast(transfer.ctx));
-        self.buffer = self.buffer_pool.get();
+        self.buffer = self.manager.buffer_pool.get();
     }
 
     fn dataCallback(transfer: *Http.Transfer, data: []const u8) !void {
@@ -822,8 +870,8 @@ const Blocking = struct {
         //     .blocking = true,
         // });
 
-        var self: *Blocking = @ptrCast(@alignCast(transfer.ctx));
-        self.buffer.appendSlice(self.allocator, data) catch |err| {
+        var self: *SyncModule = @ptrCast(@alignCast(transfer.ctx));
+        self.buffer.appendSlice(self.manager.allocator, data) catch |err| {
             log.err(.http, "SM.dataCallback", .{
                 .err = err,
                 .len = data.len,
@@ -835,19 +883,17 @@ const Blocking = struct {
     }
 
     fn doneCallback(ctx: *anyopaque) !void {
-        var self: *Blocking = @ptrCast(@alignCast(ctx));
-        self.state = .{ .done = .{
-            .buffer = self.buffer,
-            .buffer_pool = self.buffer_pool,
-        } };
+        var self: *SyncModule = @ptrCast(@alignCast(ctx));
+        self.finished(.done);
     }
 
     fn errorCallback(ctx: *anyopaque, err: anyerror) void {
-        var self: *Blocking = @ptrCast(@alignCast(ctx));
-        self.state = .{ .err = err };
-        if (self.buffer.items.len > 0) {
-            self.buffer_pool.release(self.buffer);
-        }
+        var self: *SyncModule = @ptrCast(@alignCast(ctx));
+        self.finished(.{ .err = err });
+    }
+
+    fn finished(self: *SyncModule, state: State) void {
+        self.state = state;
     }
 };
 

src/browser/page.zig

@@ -143,7 +143,7 @@ pub const Page = struct {
             .main_context = undefined,
         };
 
-        self.main_context = try session.executor.createJsContext(&self.window, self, self, true, Env.GlobalMissingCallback.init(&self.polyfill_loader));
+        self.main_context = try session.executor.createJsContext(&self.window, self, &self.script_manager, true, Env.GlobalMissingCallback.init(&self.polyfill_loader));
         try polyfill.preload(self.arena, self.main_context);
 
         try self.scheduler.add(self, runMicrotasks, 5, .{ .name = "page.microtasks" });
@@ -255,16 +255,6 @@ pub const Page = struct {
         try Node.prepend(head, &[_]Node.NodeOrText{.{ .node = parser.elementToNode(base) }});
     }
 
-    pub fn fetchModuleSource(ctx: *anyopaque, url: [:0]const u8) !ScriptManager.GetResult {
-        const self: *Page = @ptrCast(@alignCast(ctx));
-        return self.script_manager.blockingGet(url);
-    }
-
-    pub fn fetchAsyncModuleSource(ctx: *anyopaque, url: [:0]const u8, cb: ScriptManager.AsyncModule.Callback, cb_data: *anyopaque) !void {
-        const self: *Page = @ptrCast(@alignCast(ctx));
-        return self.script_manager.getAsyncModule(url, cb, cb_data);
-    }
-
     pub fn wait(self: *Page, wait_ms: i32) Session.WaitResult {
         return self._wait(wait_ms) catch |err| {
             switch (err) {

src/cdp/cdp.zig

@@ -693,7 +693,7 @@ const IsolatedWorld = struct {
         _ = try self.executor.createJsContext(
             &page.window,
             page,
-            {},
+            null,
             false,
             Env.GlobalMissingCallback.init(&self.polyfill_loader),
         );

src/http/Client.zig

@@ -86,9 +86,6 @@ allocator: Allocator,
 // request. These wil come and go with each request.
 transfer_pool: std.heap.MemoryPool(Transfer),
 
-// see ScriptManager.blockingGet
-blocking: Handle,
-
 // To notify registered subscribers of events, the browser sets/nulls this for us.
 notification: ?*Notification = null,
 
@@ -121,16 +118,12 @@ pub fn init(allocator: Allocator, ca_blob: ?c.curl_blob, opts: Http.Opts) !*Clie
     var handles = try Handles.init(allocator, client, ca_blob, &opts);
     errdefer handles.deinit(allocator);
 
-    var blocking = try Handle.init(client, ca_blob, &opts);
-    errdefer blocking.deinit();
-
     client.* = .{
         .queue = .{},
         .active = 0,
         .intercepted = 0,
         .multi = multi,
         .handles = handles,
-        .blocking = blocking,
         .allocator = allocator,
         .http_proxy = opts.http_proxy,
         .user_agent = opts.user_agent,
@@ -142,7 +135,6 @@ pub fn init(allocator: Allocator, ca_blob: ?c.curl_blob, opts: Http.Opts) !*Clie
 
 pub fn deinit(self: *Client) void {
     self.abort();
-    self.blocking.deinit();
     self.handles.deinit(self.allocator);
 
     _ = c.curl_multi_cleanup(self.multi);
@@ -263,12 +255,6 @@ pub fn fulfillTransfer(self: *Client, transfer: *Transfer, status: u16, headers:
     return transfer.fulfill(status, headers, body);
 }
 
-// See ScriptManager.blockingGet
-pub fn blockingRequest(self: *Client, req: Request) !void {
-    const transfer = try self.makeTransfer(req);
-    return self.makeRequest(&self.blocking, transfer);
-}
-
 fn makeTransfer(self: *Client, req: Request) !*Transfer {
     errdefer req.headers.deinit();
 
@@ -329,7 +315,6 @@ pub fn changeProxy(self: *Client, proxy: [:0]const u8) !void {
     for (self.handles.handles) |*h| {
         try errorCheck(c.curl_easy_setopt(h.conn.easy, c.CURLOPT_PROXY, proxy.ptr));
     }
-    try errorCheck(c.curl_easy_setopt(self.blocking.conn.easy, c.CURLOPT_PROXY, proxy.ptr));
 }
 
 // Same restriction as changeProxy. Should be ok since this is only called on
@@ -341,7 +326,6 @@ pub fn restoreOriginalProxy(self: *Client) !void {
     for (self.handles.handles) |*h| {
         try errorCheck(c.curl_easy_setopt(h.conn.easy, c.CURLOPT_PROXY, proxy));
     }
-    try errorCheck(c.curl_easy_setopt(self.blocking.conn.easy, c.CURLOPT_PROXY, proxy));
 }
 
 fn makeRequest(self: *Client, handle: *Handle, transfer: *Transfer) !void {
@@ -504,7 +488,7 @@ fn endTransfer(self: *Client, transfer: *Transfer) void {
         log.fatal(.http, "Failed to remove handle", .{ .err = err });
     };
 
-    self.handles.release(self, handle);
+    self.handles.release(handle);
     transfer._handle = null;
     self.active -= 1;
 }
@@ -563,13 +547,7 @@ const Handles = struct {
         return null;
     }
 
-    fn release(self: *Handles, client: *Client, handle: *Handle) void {
-        if (handle == &client.blocking) {
-            // the handle we've reserved for blocking request doesn't participate
-            // int he in_use/available pools
-            return;
-        }
-
+    fn release(self: *Handles, handle: *Handle) void {
         var node = &handle.node;
         self.in_use.remove(node);
         node.prev = null;
@@ -747,7 +725,7 @@ pub const Transfer = struct {
     fn deinit(self: *Transfer) void {
         self.req.headers.deinit();
         if (self._handle) |handle| {
-            self.client.handles.release(self.client, handle);
+            self.client.handles.release(handle);
         }
         self.arena.deinit();
         self.client.transfer_pool.destroy(self);