fix(ext/node): port internal/priority_queue and expose it via require#33696
Conversation
66221e1 to
4e56710
Compare
|
Force-pushed |
nathanwhitbot
left a comment
There was a problem hiding this comment.
Self-review of `4e5671043` (post-cleanup):
`internal/priority_queue.ts` port parity with Node (`lib/internal/priority_queue.js`):
Method-by-method comparison against `lib/internal/priority_queue.js`:
| Method | Node | Ours |
|---|---|---|
| Default `#compare` | `(a, b) => a - b` | Same ✓ |
| `#heap` initial | `[undefined, undefined]` (1-indexed heap) | Same ✓ |
| `insert(value)` | `heap[++this.#size] = value; percolateUp(pos)` | Same ✓ |
| `peek()` | `this.#heap[1]` | Same ✓ |
| `peekBottom()` | `this.#heap[this.#size]` | Same ✓ |
| `percolateDown(pos)` | binary-heap sift-down with optional `setPosition` callback | Same ✓ |
| `percolateUp(pos)` | binary-heap sift-up with optional `setPosition` callback | Same ✓ |
| `removeAt(pos)` | `heap[pos] = heap[size]; heap[size] = undefined; --size; percolateUp/Down` | Same ✓ |
| `shift()` | `removeAt(1); return heap[1]` (saved before remove) | Same ✓ |
The `setPosition(node, pos)` callback contract: Node uses this from `internal/timers.js` to record `node.priorityQueuePosition = pos` so it can later `removeAt(node.priorityQueuePosition)` in O(log n) without scanning. The TypeScript port preserves the contract exactly — `hasSetPosition` short-circuit is identical, and every position-mutating path (sift-up, sift-down, swap, percolate) calls `setPosition` at the same points Node does.
`module.exports = class` vs `export default class`:
Node module shape: `module.exports = class PriorityQueue { ... }`. Our TS shape: `export class PriorityQueue { ... }` + (implicit default through the require map entry). `require('internal/priority_queue')` resolves to the class itself, matching Node's `const PriorityQueue = require('internal/priority_queue')` callsite.
`01_require.js` + `lib.rs` wiring:
- Import + `setupBuiltinModules` map entry mirror `internal/event_target` / `internal/fs/utils` adjacent entries. ✓
- `lib.rs` ESM listing: `"internal/priority_queue.ts"` alphabetically positioned between `"internal/primordials.mjs"` and `"internal/process/per_thread.mjs"`. ✓
Test enrollment: `parallel/test-priority-queue.js` enabled with `{}`. ✓
LGTM, holding to COMMENT until CI completes.
nathanwhitbot
left a comment
There was a problem hiding this comment.
Single commit, no scope-creep. The PriorityQueue class is a verbatim port of Node's lib/internal/priority_queue.js:
- Same private fields (
#compare,#heap,#setPosition,#size). - Identical algorithm in
insert/peek/peekBottom/percolateDown/percolateUp/removeAt/shift. heap[0]andheap[1]initialized toundefined(1-indexed binary heap, root at index 1).
TypeScript types are accurate (Comparator<T>, SetPosition<T>, generic T = any). export default PriorityQueue is the right shape so require('internal/priority_queue') matches Node's module.exports = class PriorityQueue. lib.rs and 01_require.js mappings match the existing internal-builtin pattern.
LGTM.
fibibot
left a comment
There was a problem hiding this comment.
LGTM. Direct port of Node's lib/internal/priority_queue.js with TypeScript types layered on. Walked the algorithm side-by-side against upstream:
- Heap layout: 1-indexed, slots
[0]and[1]start asundefined(Node uses[undefined, undefined], port matches).peek()returns#heap[1],peekBottom()returns#heap[#size]. ✓ insert/percolateUp: increment size, place at end, bubble up while parent (pos >> 1) compares greater. Identical to Node. ✓shift/removeAt/percolateDown: move last element to vacated slot, decrement size, decide percolate-up vs percolate-down based on parent comparison. The two-child comparison (if (nextChild <= size && compare(heap[nextChild], childItem) < 0)) and short-circuit oncompare(item, childItem) <= 0are byte-identical to Node. ✓setPositioncallback: samehasSetPositioncached-bool optimization to avoid repeatedly checkingsetPosition !== undefinedinside the hot loops. Node's timer scheduler depends on this forremoveAt(node.priorityQueuePosition)in O(log n). ✓
Module wiring:
export default PriorityQueue+import internalPriorityQueue from "ext:deno_node/internal/priority_queue.ts"in01_require.jscorrectly mirrors Node'smodule.exports = class PriorityQueue—require('internal/priority_queue')returns the class itself. ✓- Module registered in both
01_require.js(setupBuiltinModules) andlib.rs(extension manifest). // deno-lint-ignore-file no-explicit-anyis appropriate —T = anydefault mirrors the untyped JS source.
CI: 122 success, 0 failure, 10 pending. All 36 test node_compat shards (3 × 6 platforms × debug+release) green. The pending shards are non-node_compat (likely integration/specs stragglers).
The PR body's "Doesn't conflict with parallel/test-internal-modules.js" note checks out — that test specifically asserts require('internal/freelist') still throws, which this PR doesn't touch.
|
|
Summary
require('internal/priority_queue')was unmapped — Deno had no polyfill for it. Portlib/internal/priority_queue.js(the binary heap backing Node's timer scheduler) to TypeScript, default-export the class to matchmodule.exports = class PriorityQueue, and wire it into01_require.jspluslib.rs.The class is self-contained: a binary heap accepting an optional comparator and a
setPositioncallback (Node uses the latter for timer-list bookkeeping so it canremoveAt(node.priorityQueuePosition)in O(log n)).Enables
parallel/test-priority-queue.js. Doesn't conflict withparallel/test-internal-modules.js, which only asserts thatrequire('internal/freelist')still throws.Test plan
cargo test --test node_compat -- test-priority-queue test-internal-modules(both pass)cargo test --test node_compaton Linux — only pre-existing flaky failures (test-process-threadCpuUsage-worker-threads.js,test-fs-promises-file-handle-readFile.js,test-dgram-send-cb-quelches-error.js,test-child-process-uid-gid.js,test-net-autoselectfamily.js).