Merge pull request #687 from lightpanda-io/always_gc_hints

Remove --gc_hints option, apply the --gc_hints behavior by default

Author: karlseguin

.github/workflows/e2e-test.yml

@@ -88,7 +88,7 @@ jobs:
       - name: run puppeteer
         run: |
           python3 -m http.server 1234 -d ./public & echo $! > PYTHON.pid
-          ./lightpanda serve --gc_hints & echo $! > LPD.pid
+          ./lightpanda serve & echo $! > LPD.pid
           RUNS=100 npm run bench-puppeteer-cdp > puppeteer.out || exit 1
           cat /proc/`cat LPD.pid`/status |grep VmHWM|grep -oP '\d+' > LPD.VmHWM
           kill `cat LPD.pid` `cat PYTHON.pid`

src/app.zig

@@ -28,7 +28,6 @@ pub const App = struct {
 
     pub const Config = struct {
         run_mode: RunMode,
-        gc_hints: bool = false,
         tls_verify_host: bool = true,
         http_proxy: ?std.Uri = null,
     };

src/browser/browser.zig

@@ -86,9 +86,7 @@ pub const Browser = struct {
             session.deinit();
             self.session = null;
             _ = self.session_arena.reset(.{ .retain_with_limit = 1 * 1024 * 1024 });
-            if (self.app.config.gc_hints) {
-                self.env.lowMemoryNotification();
-            }
+            self.env.lowMemoryNotification();
         }
     }
 

src/main.zig

@@ -70,7 +70,6 @@ pub fn main() !void {
 
     var app = try App.init(alloc, .{
         .run_mode = args.mode,
-        .gc_hints = args.gcHints(),
         .http_proxy = args.httpProxy(),
         .tls_verify_host = args.tlsVerifyHost(),
     });
@@ -129,13 +128,6 @@ const Command = struct {
     mode: Mode,
     exec_name: []const u8,
 
-    fn gcHints(self: *const Command) bool {
-        return switch (self.mode) {
-            .serve => |opts| opts.gc_hints,
-            else => false,
-        };
-    }
-
     fn tlsVerifyHost(self: *const Command) bool {
         return switch (self.mode) {
             inline .serve, .fetch => |opts| opts.tls_verify_host,
@@ -161,7 +153,6 @@ const Command = struct {
         host: []const u8,
         port: u16,
         timeout: u16,
-        gc_hints: bool,
         tls_verify_host: bool,
         http_proxy: ?std.Uri,
     };
@@ -210,9 +201,6 @@ const Command = struct {
             \\--timeout       Inactivity timeout in seconds before disconnecting clients
             \\                Defaults to 3 (seconds)
             \\
-            \\--gc_hints      Encourage V8 to cleanup garbage for each new browser context.
-            \\                Defaults to false
-            \\
             \\--insecure_disable_tls_host_verification
             \\                Disables host verification on all HTTP requests.
             \\                This is an advanced option which should only be
@@ -296,10 +284,6 @@ fn inferMode(opt: []const u8) ?App.RunMode {
         return .serve;
     }
 
-    if (std.mem.eql(u8, opt, "--gc_hints")) {
-        return .serve;
-    }
-
     return null;
 }
 
@@ -310,7 +294,6 @@ fn parseServeArgs(
     var host: []const u8 = "127.0.0.1";
     var port: u16 = 9222;
     var timeout: u16 = 3;
-    var gc_hints = false;
     var tls_verify_host = true;
     var http_proxy: ?std.Uri = null;
 
@@ -355,11 +338,6 @@ fn parseServeArgs(
             continue;
         }
 
-        if (std.mem.eql(u8, "--gc_hints", opt)) {
-            gc_hints = true;
-            continue;
-        }
-
         if (std.mem.eql(u8, "--http_proxy", opt)) {
             const str = args.next() orelse {
                 log.err("--http_proxy argument requires an value", .{});
@@ -377,7 +355,6 @@ fn parseServeArgs(
         .host = host,
         .port = port,
         .timeout = timeout,
-        .gc_hints = gc_hints,
         .http_proxy = http_proxy,
         .tls_verify_host = tls_verify_host,
     };

src/runtime/js.zig

@@ -269,10 +269,9 @@ pub fn Env(comptime State: type, comptime WebApis: type) type {
         }
 
         // V8 doesn't immediately free memory associated with
-        // a Context, it's managed by the garbage collector. So, when the
-        // `gc_hints` option is enabled, we'll use the `lowMemoryNotification`
-        // call on the isolate to encourage v8 to free any contexts which
-        // have been freed.
+        // a Context, it's managed by the garbage collector. We use the
+        // `lowMemoryNotification` call on the isolate to encourage v8 to free
+        // any contexts which have been freed.
         pub fn lowMemoryNotification(self: *Self) void {
             var handle_scope: v8.HandleScope = undefined;
             v8.HandleScope.init(&handle_scope, self.isolate);