Try bringing release/acquire heap access back (#16865)

Co-authored-by: Ben Grant <[email protected]>

Author: Jarred-Sumner

packages/bun-usockets/src/eventing/epoll_kqueue.c

@@ -247,7 +247,7 @@ void us_loop_run(struct us_loop_t *loop) {
     }
 }
 
-extern int Bun__JSC_onBeforeWait(void*);
+extern void Bun__JSC_onBeforeWait(void*);
 extern void Bun__JSC_onAfterWait(void*);
 
 void us_loop_run_bun_tick(struct us_loop_t *loop, const struct timespec* timeout) {
@@ -265,10 +265,8 @@ void us_loop_run_bun_tick(struct us_loop_t *loop, const struct timespec* timeout
     /* Emit pre callback */
     us_internal_loop_pre(loop);
 
-    int needs_after_wait = 0;
-    if (loop->data.jsc_vm) {
-        needs_after_wait = Bun__JSC_onBeforeWait(loop->data.jsc_vm);
-    }
+    /* Safe if jsc_vm is NULL */
+    Bun__JSC_onBeforeWait(loop->data.jsc_vm);
 
     /* Fetch ready polls */
 #ifdef LIBUS_USE_EPOLL
@@ -280,9 +278,7 @@ void us_loop_run_bun_tick(struct us_loop_t *loop, const struct timespec* timeout
     } while (IS_EINTR(loop->num_ready_polls));
 #endif
 
-    if (needs_after_wait) {
-        Bun__JSC_onAfterWait(loop->data.jsc_vm);
-    }
+    Bun__JSC_onAfterWait(loop->data.jsc_vm);
 
     /* Iterate ready polls, dispatching them by type */
     for (loop->current_ready_poll = 0; loop->current_ready_poll < loop->num_ready_polls; loop->current_ready_poll++) {

src/bun.js/api/Timer.zig

@@ -958,6 +958,10 @@ pub const WTFTimer = struct {
         return this;
     }
 
+    pub export fn WTFTimer__runIfImminent(vm: *VirtualMachine) void {
+        vm.eventLoop().runImminentGCTimer();
+    }
+
     pub fn run(this: *WTFTimer, vm: *VirtualMachine) void {
         if (this.event_loop_timer.state == .ACTIVE) {
             vm.timer.remove(&this.event_loop_timer);

src/bun.js/bindings/BunJSCEventLoop.cpp

@@ -1,20 +1,33 @@
 #include "root.h"
+#include "BunClientData.h"
 
 #include <JavaScriptCore/VM.h>
 #include <JavaScriptCore/Heap.h>
 
-extern "C" int Bun__JSC_onBeforeWait(JSC::VM* vm)
+// It would be nicer to construct a DropAllLocks in us_loop_run_bun_tick (the only function that
+// uses onBeforeWait and onAfterWait), but that code is in C. We use an optional as that lets us
+// check whether it's initialized.
+static thread_local std::optional<JSC::JSLock::DropAllLocks> drop_all_locks { std::nullopt };
+
+extern "C" void WTFTimer__runIfImminent(void* bun_vm);
+
+// Safe if VM is nullptr
+extern "C" void Bun__JSC_onBeforeWait(JSC::VM* vm)
 {
-    (void)vm;
-    // if (vm->heap.hasAccess()) {
-    //     vm->heap.releaseAccess();
-    //     return 1;
-    // }
-    return 0;
+    ASSERT(!drop_all_locks.has_value());
+    if (vm) {
+        bool previouslyHadAccess = vm->heap.hasHeapAccess();
+        drop_all_locks.emplace(*vm);
+        if (previouslyHadAccess) {
+            vm->heap.releaseAccess();
+        }
+    }
 }
 
 extern "C" void Bun__JSC_onAfterWait(JSC::VM* vm)
 {
-    (void)vm;
-    // vm->heap.acquireAccess();
+    if (vm) {
+        vm->heap.acquireAccess();
+        drop_all_locks.reset();
+    }
 }

src/bun.js/bindings/BunProcess.cpp

@@ -1068,10 +1068,18 @@ static void onDidChangeListeners(EventEmitter& eventEmitter, const Identifier& e
         }
 
         if (auto signalNumber = signalNameToNumberMap->get(eventName.string())) {
-#if !OS(WINDOWS)
+#if OS(LINUX)
+            // SIGKILL and SIGSTOP cannot be handled, and JSC needs its own signal handler to
+            // suspend and resume the JS thread which we must not override.
+            if (signalNumber != SIGKILL && signalNumber != SIGSTOP && signalNumber != g_wtfConfig.sigThreadSuspendResume) {
+#elif OS(DARWIN)
+            // these signals cannot be handled
             if (signalNumber != SIGKILL && signalNumber != SIGSTOP) {
+#elif OS(WINDOWS)
+            // windows has no SIGSTOP
+            if (signalNumber != SIGKILL) {
 #else
-            if (signalNumber != SIGKILL) { // windows has no SIGSTOP
+#error unknown OS
 #endif
 
                 if (isAdded) {

src/bun.js/bindings/ZigGlobalObject.cpp

@@ -161,6 +161,7 @@
 #include "JSX509Certificate.h"
 #include "JSS3File.h"
 #include "S3Error.h"
+#include <JavaScriptCore/JSBasePrivate.h>
 #if ENABLE(REMOTE_INSPECTOR)
 #include "JavaScriptCore/RemoteInspectorServer.h"
 #endif
@@ -236,6 +237,22 @@ extern "C" void JSCInitialize(const char* envp[], size_t envc, void (*onCrash)(c
         return;
     has_loaded_jsc = true;
     JSC::Config::enableRestrictedOptions();
+#if OS(LINUX)
+    {
+        // By default, JavaScriptCore's garbage collector sends SIGUSR1 to the JS thread to suspend
+        // and resume it in order to scan its stack memory. Whatever signal it uses can't be
+        // reliably intercepted by JS code, and several npm packages use SIGUSR1 for various
+        // features. We tell it to use SIGPWR instead, which we assume is unlikely to be reliable
+        // for its stated purpose. Mono's garbage collector also uses SIGPWR:
+        // https://www.mono-project.com/docs/advanced/embedding/#signal-handling
+        //
+        // This call needs to be before most of the other JSC initialization, as we can't
+        // reconfigure which signal is used once the signal handler has already been registered.
+        bool configure_signal_success = JSConfigureSignalForGC(SIGPWR);
+        ASSERT(configure_signal_success);
+        ASSERT(g_wtfConfig.sigThreadSuspendResume == SIGPWR);
+    }
+#endif
 
     std::set_terminate([]() { Zig__GlobalObject__onCrash(); });
     WTF::initializeMainThread();

src/bun.js/bindings/bindings.zig

@@ -6209,25 +6209,6 @@ pub const VM = extern struct {
         LargeHeap = 1,
     };
 
-    extern fn Bun__JSC_onBeforeWait(vm: *VM) i32;
-    extern fn Bun__JSC_onAfterWait(vm: *VM) void;
-    pub const ReleaseHeapAccess = struct {
-        vm: *VM,
-        needs_to_acquire: bool,
-        pub fn acquire(this: *const ReleaseHeapAccess) void {
-            if (this.needs_to_acquire) {
-                Bun__JSC_onAfterWait(this.vm);
-            }
-        }
-    };
-
-    /// Temporarily give up access to the heap, allowing other work to proceed. Call acquire() on
-    /// the return value at scope exit. If you did not already have heap access, release and acquire
-    /// are both safe no-ops.
-    pub fn releaseHeapAccess(vm: *VM) ReleaseHeapAccess {
-        return .{ .vm = vm, .needs_to_acquire = Bun__JSC_onBeforeWait(vm) != 0 };
-    }
-
     pub fn create(heap_type: HeapType) *VM {
         return cppFn("create", .{@intFromEnum(heap_type)});
     }

src/bun.js/event_loop.zig

@@ -1410,6 +1410,12 @@ pub const EventLoop = struct {
         }
     }
 
+    pub fn runImminentGCTimer(this: *EventLoop) void {
+        if (this.imminent_gc_timer.swap(null, .seq_cst)) |timer| {
+            timer.run(this.virtual_machine);
+        }
+    }
+
     pub fn tickConcurrentWithCount(this: *EventLoop) usize {
         this.updateCounts();
 
@@ -1419,9 +1425,7 @@ pub const EventLoop = struct {
             }
         }
 
-        if (this.imminent_gc_timer.swap(null, .seq_cst)) |timer| {
-            timer.run(this.virtual_machine);
-        }
+        this.runImminentGCTimer();
 
         var concurrent = this.concurrent_tasks.popBatch();
         const count = concurrent.count;
@@ -1492,9 +1496,7 @@ pub const EventLoop = struct {
             }
         }
 
-        if (this.imminent_gc_timer.swap(null, .seq_cst)) |timer| {
-            timer.run(ctx);
-        }
+        this.runImminentGCTimer();
 
         if (loop.isActive()) {
             this.processGCTimer();

src/bun.js/module_loader.zig

@@ -1660,12 +1660,6 @@ pub const ModuleLoader = struct {
                 var parse_result: ParseResult = switch (disable_transpilying or
                     (loader == .json and !path.isJSONCFile())) {
                     inline else => |return_file_only| brk: {
-                        const heap_access = if (!disable_transpilying)
-                            jsc_vm.jsc.releaseHeapAccess()
-                        else
-                            JSC.VM.ReleaseHeapAccess{ .vm = jsc_vm.jsc, .needs_to_acquire = false };
-                        defer heap_access.acquire();
-
                         break :brk jsc_vm.transpiler.parseMaybeReturnFileOnly(
                             parse_options,
                             null,

src/linux_c.zig

@@ -642,6 +642,9 @@ export fn sys_epoll_pwait2(epfd: i32, events: ?[*]std.os.linux.epoll_event, maxe
             @bitCast(@as(isize, @intCast(maxevents))),
             @intFromPtr(timeout),
             @intFromPtr(sigmask),
+            // This is the correct value. glibc claims to pass `sizeof sigset_t` for this argument,
+            // which would be 128, but they actually pass 8 which is what the kernel expects.
+            // https://github.com/ziglang/zig/issues/12715
             8,
         ),
     );