perf(promise_core): migrate timer queue to @priority_queue#433
Conversation
Replace the Array[TimerTask] + sort_by + remove(0) pattern (O(n² log n) per run_timers call) with @priority_queue.PriorityQueue[TimerTask] (O(n log n) total). The priority queue is a max-heap; Compare is reversed so pop() always extracts the soonest-due timer first. Equal-delay tie-break is by insertion_order, preserving WHATWG insertion-order semantics. Cancellation moves to lazy deletion only: clearTimeout/clearInterval set cancelled_timer_ids; the drain loop skips popped timers whose id is in that set. The mut cancelled struct field is removed. The cancelled_ids map is cleared only after a full drain so that timers surviving the 10k- iteration safety limit are still correctly skipped on the next call. Microtask queue is deliberately left unchanged: the head-pointer drain is already O(n); @DeQue would be API cleanup only with no complexity win. Benchmark results (200-timer/microtask workloads): event_loop/timer_drain_200: 4.19 ms → 1.95 ms (2.15× speedup) event_loop/microtask_drain_200: unchanged within noise (control) New tests: equal-delay registration order, clearTimeout cancel, and clearInterval-stops-after-N-fires cover all three cancellation paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 46 minutes and 19 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughMigrates ChangesTimer Queue Priority Queue Migration
Sequence Diagram(s)sequenceDiagram
participant JS as JS Code
participant builtins as builtins_promise
participant host as HostEnv
participant run as run_timers
JS->>builtins: setTimeout(cb, delay)
builtins->>host: timer_queue.push(TimerTask{id, delay, ...})
JS->>builtins: clearTimeout(id)
builtins->>host: cancelled_timer_ids[id] = true
run->>host: timer_queue.pop() → TimerTask
run->>host: cancelled_timer_ids.contains(timer.id)?
alt cancelled
run->>host: skip, pop next
else not cancelled
run->>run: execute callback
run->>run: drain microtasks
alt setInterval and not cancelled
run->>host: timer_queue.push(re-enqueued TimerTask)
end
end
alt queue empty
run->>host: clear cancelled_timer_ids
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f24707e240
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| None => break // unreachable: guarded by length() > 0 above | ||
| } | ||
| // Lazy deletion: skip timers cancelled via clearTimeout/clearInterval | ||
| if self.host.cancelled_timer_ids.contains(timer.id) { |
There was a problem hiding this comment.
When clearTimeout/clearInterval is called with an ID that is not currently queued, cancel_timer still records that ID; with this new lazy-deletion check applied to every popped timer, a fresh script like clearTimeout(1); setTimeout(() => console.log("ran"), 0) skips the later timer because the first allocated ID is also 1. Clearing an unknown handle should be a no-op, so the cancellation set needs to be limited to pending/active IDs or stale IDs need to be removed before a newly scheduled timer can inherit them.
Useful? React with 👍 / 👎.
| ) | ||
| } | ||
| b.bench(fn() { | ||
| let _ = @lib.run(TIMER_DRAIN_200_SRC) catch { |
There was a problem hiding this comment.
This benchmark loop calls @lib.run, and that facade constructs a new interpreter and parses the source before draining timers, so event_loop/timer_drain_200 measures startup/parse work along with the priority-queue drain despite being used as the queue-drain gate. A parser/startup change can mask or fake a timer-queue regression here; use a lower-level/pre-parsed setup or relabel the benchmark before relying on these numbers for the migration.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
benchmarks/benchmarks.mbt (1)
486-497: ⚡ Quick winUse
fail(...)for benchmark defect checks instead ofabort(...).Line 486-Line 497 and Line 513-Line 524 introduce new
abort(...)checks; these should usefail(...)per project convention.As per coding guidelines: "Use
fail("msg")for defect detection instead ofabort()— it is catchable and provides source location."Also applies to: 513-524
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/benchmarks.mbt` around lines 486 - 497, Replace all `abort(...)` calls with `fail(...)` throughout the benchmark defect checks in the MICROTASK_DRAIN_200 benchmark test. This includes the pre-check error handling at line 486 (the catch block after the pre-check), the correctness check failure at line 489-492 (the abort call with the expected vs got comparison message), and the bench loop error handling at line 496 (the catch block within b.bench). Also apply the same change to the other benchmark test mentioned in the same range (lines 513-524). The `fail(...)` function should be used instead of `abort(...)` per project convention as it is catchable and provides source location information.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/benchmarks.mbt`:
- Around line 504-510: The benchmark description for run_timers() still
documents the old sort_by + remove(0) implementation with O(n² log n)
complexity, but this has been migrated to use a priority queue. Update the
comment block starting at line 504 to describe the current priority_queue
behavior and complexity characteristics, optionally noting the old behavior as
historical context to help readers understand the migration context referenced
in issue `#329`.
In `@benchmarks/workloads.mbt`:
- Around line 745-748: The documentation comment for TIMER_DRAIN_200_SRC is
outdated and describes the old O(n² log n) complexity based on sort_by and
remove(0) operations, but the run_timers() implementation has been migrated to
use a priority queue. Update the comment block preceding the TIMER_DRAIN_200_SRC
constant to accurately reflect the current priority-queue based implementation
and its actual complexity characteristics, replacing references to sort_by and
remove(0) with the appropriate priority queue operations.
---
Nitpick comments:
In `@benchmarks/benchmarks.mbt`:
- Around line 486-497: Replace all `abort(...)` calls with `fail(...)`
throughout the benchmark defect checks in the MICROTASK_DRAIN_200 benchmark
test. This includes the pre-check error handling at line 486 (the catch block
after the pre-check), the correctness check failure at line 489-492 (the abort
call with the expected vs got comparison message), and the bench loop error
handling at line 496 (the catch block within b.bench). Also apply the same
change to the other benchmark test mentioned in the same range (lines 513-524).
The `fail(...)` function should be used instead of `abort(...)` per project
convention as it is catchable and provides source location information.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c167e684-acd2-486c-8bd7-87ea63389de9
📒 Files selected for processing (11)
benchmarks/benchmarks.mbtbenchmarks/moon.pkgbenchmarks/pkg.generated.mbtibenchmarks/startup_new_interpreter_subphases.mbtbenchmarks/workloads.mbtinterpreter/interpreter_test.mbtinterpreter/runtime/interpreter.mbtinterpreter/runtime/moon.pkginterpreter/runtime/pkg.generated.mbtiinterpreter/runtime/promise_core.mbtinterpreter/stdlib/builtins_promise.mbt
Benchmark ResultsRun: https://github.com/dowdiness/js_engine/actions/runs/27904310137
Stage summary
Focused bytecode base-vs-head comparison
Base-vs-head comparison
Mean-time chart (log scale)
Closure-conversion comparison
|
… review fixups
- schedule_timer now calls cancelled_timer_ids.remove(id) before pushing
the new timer: clearTimeout/clearInterval called with IDs not yet in the
queue (or never valid) can no longer silently cancel the first timer to
inherit that ID. Test: "clearTimeout with unknown id is a no-op".
- Update event_loop/timer_drain_200 benchmark comment: was still describing
the old sort_by + remove(0) / O(n² log n) path; now documents priority-
queue pop() / O(n log n) and notes that @lib.run includes parse+startup
overhead (so this is a throughput regression gate, not an isolated
queue-op microbenchmark).
- Update TIMER_DRAIN_200_SRC workloads comment to match (same stale
O(n² log n) language removed).
Skipped: abort→fail nitpick in benchmarks.mbt — the file has a deliberate
comment at line 41 ("Uses abort() (always fatal) so it works in any error
context") and all 15+ existing bench tests follow this pattern; changing
only the two new tests would be inconsistent.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d613b17715
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Clear cancelled IDs only when the queue is fully drained. | ||
| // If the safety limit fires, surviving timers may fire on the next | ||
| // run_timers call; keeping cancelled IDs ensures they are still skipped. | ||
| if self.host.timer_queue.length() == 0 { |
There was a problem hiding this comment.
When an interval keeps timer_queue non-empty (or the 10k safety limit is hit), this conditional means the cancellation map may never be cleared. Since consumed IDs are also left in the map when a canceled timer is skipped or a self-canceled interval is not re-enqueued, long-lived hosts that repeatedly cancel timers while another interval remains active accumulate stale IDs unboundedly; remove IDs when their cancellation is consumed, or otherwise clear known-dead entries.
Useful? React with 👍 / 👎.
1 participant
Closes #329.
What changed
Replaces
Array[TimerTask]+sort_by+remove(0)inrun_timers(O(n² log n) per invocation) with@priority_queue.PriorityQueue[TimerTask](O(n log n) total drain).Heap direction.
@priority_queueis a max-heap.Compareis inverted — smaller(delay, insertion_order)compares as "greater" — sopop()always extracts the soonest-due timer. This preserves the WHATWG insertion-order tie-break that the existing equal-delay test pins.Cancellation. The old code had a dual path: mutate
timer.cancelled = truein-queue AND record incancelled_timer_ids. WithPriorityQueue, in-heap mutation is impossible.mut cancelledis removed fromTimerTask; all cancellation goes throughcancelled_timer_ids(lazy deletion). The drain loop skips any popped timer whose id is present in the set. The id set is cleared only after a full drain, so timers that survive the 10k-iteration safety limit are still correctly skipped on the nextrun_timerscall.Microtask queue deliberately unchanged. The head-pointer drain in
run_microtasksis already O(n). Migrating to@dequewould be API cleanup with no complexity benefit and is out of scope here.Benchmark results
200-timer workload (
event_loop/timer_drain_200):The microtask bench is the control: it has exactly one
setTimeoutsentinel, so array vs heap makes no O(n) difference — the flat result confirms only the timer path was affected.Tests added
timers with equal delay fire in registration order— WHATWG tie-break invariantclearTimeout cancels timer before it fires— lazy-deletion path for clearTimeoutclearInterval inside callback stops re-enqueue— cancellation during interval callbackFiles changed
interpreter/runtime/interpreter.mbtmut cancelled: BoolfromTimerTask; changetimer_queuefield type toPriorityQueue[TimerTask]interpreter/runtime/promise_core.mbtpub impl Eq/CompareforTimerTask; rewriterun_timersdrain loopinterpreter/stdlib/builtins_promise.mbtcancelled: falsefrom struct literal; simplifycancel_timerto lazy-deletion-onlyinterpreter/runtime/moon.pkg@priority_queueimportbenchmarks/@priority_queueimport,event_loop/*workload constants, two benchmark testsinterpreter/interpreter_test.mbt*.mbtimoon info🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests