Use stopIfNecessary() instead of heap.{acquireAccess,releaseAccess} (#21598)

### What does this PR do?

dropAllLocks causes Thread::yield several times which means more system
calls which means more thread switches which means slower

### How did you verify your code works?
This commit is contained in:
Jarred Sumner
2025-08-04 00:45:33 -07:00
committed by GitHub
parent 1ac2391b20
commit b6d3768038
2 changed files with 57 additions and 79 deletions

View File

@@ -246,8 +246,7 @@ void us_loop_run(struct us_loop_t *loop) {
}
}
extern int Bun__JSC_onBeforeWait(void*);
extern void Bun__JSC_onAfterWait(void*);
extern void Bun__JSC_onBeforeWait(void * _Nonnull jsc_vm);
void us_loop_run_bun_tick(struct us_loop_t *loop, const struct timespec* timeout) {
if (loop->num_polls == 0)
@@ -264,8 +263,9 @@ void us_loop_run_bun_tick(struct us_loop_t *loop, const struct timespec* timeout
/* Emit pre callback */
us_internal_loop_pre(loop);
/* Safe if jsc_vm is NULL */
int must_call_on_after_wait = Bun__JSC_onBeforeWait(loop->data.jsc_vm);
if (loop->data.jsc_vm)
Bun__JSC_onBeforeWait(loop->data.jsc_vm);
/* Fetch ready polls */
#ifdef LIBUS_USE_EPOLL
@@ -276,9 +276,6 @@ void us_loop_run_bun_tick(struct us_loop_t *loop, const struct timespec* timeout
} while (IS_EINTR(loop->num_ready_polls));
#endif
if (must_call_on_after_wait) {
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++) {

View File

@@ -4,83 +4,64 @@
#include <JavaScriptCore/VM.h>
#include <JavaScriptCore/Heap.h>
// 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" int Bun__JSC_onBeforeWait(JSC::VM* vm)
extern "C" void Bun__JSC_onBeforeWait(JSC::VM* _Nonnull vm)
{
ASSERT(!drop_all_locks.has_value());
if (vm) {
bool previouslyHadAccess = vm->heap.hasHeapAccess();
// sanity check for debug builds to ensure we're not doing a
// use-after-free here
ASSERT(vm->refCount() > 0);
if (previouslyHadAccess) {
ASSERT(vm);
const bool previouslyHadAccess = vm->heap.hasHeapAccess();
// sanity check for debug builds to ensure we're not doing a
// use-after-free here
ASSERT(vm->refCount() > 0);
if (previouslyHadAccess) {
// Releasing heap access is a balance between:
// 1. CPU usage
// 2. Memory usage
//
// Not releasing heap access causes benchmarks like
// https://github.com/oven-sh/bun/pull/14885 to regress due to
// finalizers not being called quickly enough.
//
// Releasing heap access too often causes high idle CPU usage.
//
// For the following code:
// ```
// setTimeout(() => {}, 10 * 1000)
// ```
//
// command time -v when with defaultRemainingRunsUntilSkipReleaseAccess = 0:
//
// Involuntary context switches: 605
//
// command time -v when with defaultRemainingRunsUntilSkipReleaseAccess = 5:
//
// Involuntary context switches: 350
//
// command time -v when with defaultRemainingRunsUntilSkipReleaseAccess = 10:
//
// Involuntary context switches: 241
//
// Also comapre the #14885 benchmark with different values.
//
// The idea here is if you entered JS "recently", running any
// finalizers that might've been waiting to be run is a good idea.
// But if you haven't, like if the process is just waiting on I/O
// then don't bother.
static constexpr int defaultRemainingRunsUntilSkipReleaseAccess = 10;
// Releasing heap access is a balance between:
// 1. CPU usage
// 2. Memory usage
//
// Not releasing heap access causes benchmarks like
// https://github.com/oven-sh/bun/pull/14885 to regress due to
// finalizers not being called quickly enough.
//
// Releasing heap access too often causes high idle CPU usage.
//
// For the following code:
// ```
// setTimeout(() => {}, 10 * 1000)
// ```
//
// command time -v when with defaultRemainingRunsUntilSkipReleaseAccess = 0:
//
// Involuntary context switches: 605
//
// command time -v when with defaultRemainingRunsUntilSkipReleaseAccess = 5:
//
// Involuntary context switches: 350
//
// command time -v when with defaultRemainingRunsUntilSkipReleaseAccess = 10:
//
// Involuntary context switches: 241
//
// Also comapre the #14885 benchmark with different values.
//
// The idea here is if you entered JS "recently", running any
// finalizers that might've been waiting to be run is a good idea.
// But if you haven't, like if the process is just waiting on I/O
// then don't bother.
static constexpr int defaultRemainingRunsUntilSkipReleaseAccess = 10;
static thread_local int remainingRunsUntilSkipReleaseAccess = 0;
static thread_local int remainingRunsUntilSkipReleaseAccess = 0;
// Note: usage of `didEnterVM` in JSC::VM conflicts with Options::validateDFGClobberize
// We don't need to use that option, so it should be fine.
if (vm->didEnterVM) {
vm->didEnterVM = false;
remainingRunsUntilSkipReleaseAccess = defaultRemainingRunsUntilSkipReleaseAccess;
}
// Note: usage of `didEnterVM` in JSC::VM conflicts with Options::validateDFGClobberize
// We don't need to use that option, so it should be fine.
if (vm->didEnterVM) {
vm->didEnterVM = false;
remainingRunsUntilSkipReleaseAccess = defaultRemainingRunsUntilSkipReleaseAccess;
}
if (remainingRunsUntilSkipReleaseAccess-- > 0) {
drop_all_locks.emplace(*vm);
vm->heap.releaseAccess();
vm->didEnterVM = false;
return 1;
}
if (remainingRunsUntilSkipReleaseAccess-- > 0) {
// Constellation:
// > If you are not moving a VM to the different thread, then you can aquire the access and do not need to release
vm->heap.stopIfNecessary();
vm->didEnterVM = false;
}
}
return 0;
}
extern "C" void Bun__JSC_onAfterWait(JSC::VM* vm)
{
if (vm) {
vm->heap.acquireAccess();
drop_all_locks.reset();
}
}