[danlapid]

LockTiming used to time GC itself, through GC prologue and epilogue hooks, and only when the request was traced in Jaeger. Those hooks bracket only the atomic pause, so incremental marking steps run on the main thread went uncounted.

The isolate lock now measures GC with V8's LongTaskStats: it resets the stats when the lock is taken and reads them before release. That covers young collections, full atomic pauses and incremental marking. The lock also counts the full (mark-compact) collections that finish while it is held. Both values go to the new LockTiming::reportGc(), which replaces gcPrologue() and gcEpilogue(), and it is called on every lock, not only traced ones.

Downstream, edgeworker uses this to publish per-invocation GC time and full GC count in its invocation logs. Nothing else implements LockTiming's GC hooks.

About LongTaskStats being experimental

v8::metrics::LongTaskStats is marked experimental in v8-metrics.h, but it's well established:

• It was added in April 2021 (V8 commit 521ae93bf65, by the V8 GC team). Its API was last extended in December 2021, when v8_execute_us was added, and has only seen mechanical refactors since.
• Chromium uses it in production. Blink calls Reset() at the start of every main-thread task (PerformanceMonitor::WillProcessTask) and Get() for the PerformanceAPI.LongTask UKM (RecordLongTaskUkm in core/timing/performance.cc). Blink computes Duration.V8.GC from the same three-field sum we use. The V8 GC leads own that UKM.
• V8 tests it in cctest (LongTaskStatsFullAtomic, LongTaskStatsFullIncremental, ...).

If it is removed, the build breaks at the V8 upgrade. If it silently stops reporting, edgeworker's gc-logging test, which asserts that GC time is non-zero after forced GCs, catches it.

🤖 Generated with Claude Code

[clanky-the-clanker]

Superseded by the review of bc6c62f4.

This review, of 4104eca4

✅ Approved — No issues found: GC is measured over the whole time the lock is held, and the observer interface change is small and consistent with how it is called.

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

✅ All checks passed

📋 Code Quality: ✅ LGTM

No issues found. The LongTaskStats reset and read bracket the time the lock is held, the full-GC counter is kept per lock and incremented only for mark-compact epilogues, and WebAssembly instance cleanup in the GC prologue still works as before.

---

📝 Documentation: ✅ LGTM

No issues found. The new reportGc() doc comment and the updated comments where the callbacks are registered describe the current behaviour accurately.

---

_{Reviewed commit: 4104eca4}

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 68.31%. Comparing base (e48c3f9) to head (e777644).

Files with missing lines	Patch %	Lines
src/workerd/io/observer.h	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8099   +/-   ##
=======================================
  Coverage   68.31%   68.31%           
=======================================
  Files         726      726           
  Lines      189662   189670    +8     
  Branches    29672    29672           
=======================================
+ Hits       129567   129577   +10     
  Misses      42719    42719           
+ Partials    17376    17374    -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[uncleDecart]

lgtm, only thing I'd add is virtual destuctor to LockTiming class

[danlapid]

@uncleDecart Added in bc6c62f: virtual ~LockTiming() noexcept(false) = default;, matching the other observer interfaces in observer.h. Also rebased onto main.

[clanky-the-clanker]

Superseded by the review of e7776447.

This review, of bc6c62f4

✅ Approved — No issues found: the only change since the last review is the requested virtual destructor on LockTiming, and the GC measurement logic is still correct.

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

✅ All checks passed

📋 Code Quality: ✅ LGTM

No issues found. virtual ~LockTiming() noexcept(false) = default; is correct for an interface owned through kj::Own<LockTiming>. The LongTaskStats reset happens after the lock is acquired, and the read happens before currentLock is cleared, so it brackets the whole time the lock is held. fullGcCount only goes up for kGCTypeMarkSweepCompact. LockRecord::reportGc does nothing when there is no LockTiming. Nothing in the tree still refers to the removed hooks.

---

📝 Documentation: ✅ LGTM

No issues found. The reportGc() doc comment and the updated comments at the places where the callbacks are registered match what the code does now.

---

_{Reviewed commit: bc6c62f4}

[clanky-the-clanker]

✅ Approved — No issues found: the only change since the last review is the requested virtual destructor on LockTiming, and the GC measurement logic is still correct.

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

✅ All checks passed

📋 Code Quality: ✅ LGTM

No issues found. virtual ~LockTiming() noexcept(false) = default; is correct for an interface owned through kj::Own<LockTiming>. The LongTaskStats reset happens after the lock is acquired, and the read happens before currentLock is cleared, so it brackets the whole time the lock is held. fullGcCount only goes up for kGCTypeMarkSweepCompact. LockRecord::reportGc does nothing when there is no LockTiming. Nothing in the tree still refers to the removed hooks.

---

📝 Documentation: ✅ LGTM

No issues found. The reportGc() doc comment and the updated comments at the places where the callbacks are registered match what the code does now.

---

_{Reviewed commit: e7776447 — same changes as bc6c62f4, so this is that commit's review (no new review was run; line numbers are as of bc6c62f4)}