Here are some lightly edited generated comments:

PR #5545 Review Comments

1. finishing global vs per-task field

The var finishing bool in task_asyncify.go is a plain package-level global, justified by the cooperative scheduler guarantee that no suspension point exists between deadlock()→Pause()→Resume(). This is correct today, but fragile: if a preemption point is ever introduced in that path, two goroutines could clobber each other's flag silently.

Consider moving this to a field on *Task (or a bit in an existing field). That would make the intent explicit ("this specific task is finishing") and eliminate the implicit coupling to cooperative scheduling semantics. The cost is one extra byte or bit per task, which is negligible compared to the stack buffer it already owns.

2. Build tag coverage across scheduler variants

The PR splits gc_finalizer_sched.go into two files:

• gc_finalizer_sched.go: (gc.conservative || gc.precise) && (scheduler.tasks || scheduler.asyncify) which installs the idle hook + spawns runner
• gc_finalizer_sched_other.go: (gc.conservative || gc.precise) && !scheduler.none && !scheduler.tasks && !scheduler.asyncify which spawns runner only

The existing scheduler.none path has no spawnFinalizerRunner (it drains inline via wakeFinalizer).

Please confirm the union of these three paths is exhaustive for every scheduler variant (none, tasks, asyncify, cores, threads). In particular, verify that no future scheduler variant could fall through all three build constraints and leave spawnFinalizerRunner undefined.

3. Threshold of 32 regarding tunability and documentation

finalizerGCThreshold = 32 is described as "a policy constant, like Go's forcegcperiod." A few points worth discussing:

• Is 32 the right default? For a bursty syscall/js workload creating hundreds of js.Values per event, 32 triggers roughly one collection per event handler dispatch (at the idle point), which seems well-calibrated. But a workload that legitimately registers many long-lived finalizers in a setup phase would pay for several unnecessary collections during init.

• Should it be runtime-tunable? Go's GOGC is an environment variable; forcegcperiod is a named constant but not user-facing. If this stays a compile-time constant, documenting it in a comment (already done) is probably sufficient. If there's any chance users will need to tune it, exposing it via an environment variable or runtime/debug style API would be worth considering now rather than later.

• Zero disables the trigger this is documented in the code comment, which is good. Worth calling out in any user-facing docs if they exist.

Thanks @deadprogram, all fair points. Done:

1. finishing flag. Now a per-goroutine field on the asyncify state struct instead of a package global, so each finishing goroutine owns its own and the handoff to Resume no longer depends on scheduler timing. One byte per task, asyncify only.

2. Build tag coverage. There's a fourth file, gc_finalizer_sched_none.go (no-op for scheduler.none). The three constraints partition the scheduler space, so every variant, present or future, matches exactly one file (cores/threads land in the !none && !tasks && !asyncify catch-all). I documented the partition in a comment and added a test that iterates validSchedulerOptions, so a newly added scheduler is checked automatically.

3. Threshold of 32. Kept it a const. It only fires at an idle point and resets the counter, so a big setup burst costs one collection, not one per 32:

Happy to make it tunable via a runtime/debug setter or an env var if you'd rather; I left the API shape to you since it can be added later without breaking anything.

Any further comments from @jakebailey @dgryski or anyone else also wasm-involved?

@deadprogram I kept thinking about your question on the 32 threshold, so I went and read how upstream Go paces this. It ties collection to proportional growth (GOGC) and keeps a per-span bitmap so the collector can skip spans with nothing registered, rather than relying on a fixed count.

Borrowing both on top of what's already here:

• max(32, numFinalizers/2) as the trigger, which is the GOGC reasoning and the direct answer to your question: the cost of any fixed threshold depends on how much is registered, so a proportion travels better than a constant. 32 becomes the floor.
• a per-block registration bit, the analogue of Go's span bitmap, so a fresh object answers "already registered?" with a bit test. Lazily allocated, grown with the heap, and conservative for addresses it doesn't cover yet.

Both mechanisms are battle-tested in Go's own collector, which counts for something this central. I benchmarked the branch before and after on a WASM workload that registers finalizers heavily: the new one is consistently faster and, more importantly, stops degrading as the table grows. Reclamation behaviour is unchanged.

I also tried Go's per-span partitioning (64-way buckets) and measured it as redundant here: with the bit there's no scan left to shorten, and the partitioning mostly buys per-span locking, which doesn't apply under a global lock. Left it out.

testdata/finalizerbits.go covers clear-then-register, register-twice-replaces, register/clear churn, allocation into reused memory and a mixed batch. They pass with and without the change, by design.

Merged current dev, and TestBinarySize comes out unchanged on all three targets, so nothing to adjust there.

Hey, small ping on this one ☺️.

I've been using this branch as the WASM backend engine for a Go web framework I maintain, and it's been holding up really nicely so far. Even on the rougher stress tests (10k clicks on a page over 5 minutes) the slot count stays flat, which was exactly the thing I was hoping to fix.

No rush at all, just didn't want it to slip off the radar. Thanks for the reviews so far!

@deadprogram @jakebailey

@jakebailey any more comments on this PR?

If this code works on other platforms/schedulers than just javascript, it would be nice to have some tests demonstrating that.

@dgryski I added some tests for the other platforms.

testdata/finalizerinvariants.go runs on every scheduler now: asyncify (wasm), tasks (cortex-m-qemu), cores (riscv-qemu), plus threads and none on the host with -gc=conservative.

I kept it to assertions that hold under conservative stack scanning: a cleared finalizer never runs, a replaced one never runs the old func, a reachable object is never finalized, nothing runs twice. Those are the ones the registration bitmap can break.

What it doesn't assert is that a finalizer actually fired. That needs the dropped object to really get collected, which only holds on wasm, so the three existing tests stay wasm-only.

AVR I had to skip. Registering and clearing works fine there, but a single runtime.GC() never comes back, and gc.go is already skipped on simavr for the same reason.

As for whether other platforms actually gain anything, honestly I think it's just WASM for now. syscall/js is the only place in the tree that registers a finalizer at all. That might change though. TinyGo has its own os package, and unlike upstream Go its os.File doesn't close the fd on GC. If that ever lands, WASI would pick this up for free, since it's already on the block GC with a cooperative idle point.

Had to make a small change after the Windows CI broke: the two host scheduler variants I added don't build there. scheduler.threads needs threadID, which internal/task only defines for linux and darwin, and scheduler.none doesn't link either. Both reproduce with any testdata file, so they aren't new.

Limited those two runs to linux and darwin, same as TestTimerStopResetRace already does.

All Tests passed.

Thanks for the reviews so far @deadprogram @jakebailey @dgryski .

Let me know if you have anything else.

The gc and scheduler, if build with -tags=runtime_asserts, set gcAsserts = true and schedulerAsserts = true, which are used to validate data structures and other things to ensure there are no invalid states with their bookkeeping or usage. Are there any asserts we could add to the new finalizer code?

@dgryski Added some asserts for the finalizer bookkeeping.

The main ones check the registration bitmap against the table it stands in for: when the bit says "not registered" and the clear or replace path takes the shortcut, the assert walks the table anyway and fails if the object is actually there.

The others cover numFinalizers matching the table, every pending object surviving resurrection, and nothing being dequeued after it's been freed.

Hey all, just a friendly check-in on this one.

All review comments are addressed and CI is green. The new cross-scheduler tests cover asyncify, tasks, cores and threads, and the locking fix from the last round is verified on wasm, wasip1, microbit and pico. The runtime_asserts coverage for the finalizer bookkeeping landed as well.

This branch has been serving as the wasm backend for a framework I maintain, and the bridge table stays flat under sustained load, which was the whole point of the PR.

A fresh pass would be really welcome when you have time. Thanks for all the careful reviews so far.

@deadprogram @jakebailey @dgryski