Optimize _sortNodeUp/Down methods and _moveNode method (11x improvement)#664
Conversation
Improve performance, upgrade packages, and bump to v2.6
|
""" WalkthroughThis update primarily refactors internal methods of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Heap
participant InternalArray
User->>Heap: init(array)
Heap->>Heap: for i = lastParent down to 0
Heap->>Heap: _sortNodeDown(i)
Heap->>InternalArray: Move nodes using "hole" approach (no swaps)
Heap-->>User: Heap initialized
User->>Heap: insert(value)
Heap->>Heap: _sortNodeUp(index)
Heap->>InternalArray: Move nodes using "hole" approach (no swaps)
Heap-->>User: Value inserted
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/HeapAsync.ts (1)
730-732: Optimization approach looks good but useconstinstead oflet.The change from array destructuring to using a temporary variable for swapping elements is a good optimization that will improve performance. This is likely the source of the 11x-22x performance improvement mentioned in the PR title.
However, since
tempis never reassigned, you should useconstinstead ofletas per best practices.- let temp = this.heapArray[j]; + const temp = this.heapArray[j]; this.heapArray[j] = this.heapArray[k]; this.heapArray[k] = temp;🧰 Tools
🪛 ESLint
[error] 730-730: 'temp' is never reassigned. Use 'const' instead.
(prefer-const)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/heap-js.es5.jsis excluded by!**/dist/**dist/heap-js.umd.jsis excluded by!**/dist/**
📒 Files selected for processing (8)
docs/functions/toInt.html(1 hunks)docs/index.html(2 hunks)docs/types/AsyncComparator.html(1 hunks)docs/types/AsyncIsEqual.html(1 hunks)docs/types/Comparator.html(1 hunks)docs/types/IsEqual.html(1 hunks)src/Heap.ts(1 hunks)src/HeapAsync.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/Heap.ts
[error] 807-807: 'temp' is never reassigned. Use 'const' instead.
(prefer-const)
src/HeapAsync.ts
[error] 730-730: 'temp' is never reassigned. Use 'const' instead.
(prefer-const)
🔇 Additional comments (7)
docs/types/AsyncComparator.html (1)
1-1: Documentation update looks good.This is just an update to the generated documentation HTML with changes to the source reference URL and commit hash. No functional changes to review.
docs/types/AsyncIsEqual.html (1)
1-1: Documentation update looks good.This is just an update to the generated documentation HTML with changes to the source reference URL and commit hash. No functional changes to review.
docs/functions/toInt.html (1)
1-2: Documentation source link updated correctly.
The “Defined in” link now points to theBeatsuDev/heap-jsrepository at commitcc2e4c9b4b1032017a337f59889249bf1a9d27e5. No other content or structure was altered.docs/types/Comparator.html (1)
1-2: Updated repository and commit hash in source reference.
The “Defined in” link forComparatornow correctly references the new fork and commit. No functional documentation changes detected.docs/types/IsEqual.html (1)
1-2: Source link adjusted to new commit.
The “Defined in” reference forIsEqualnow points tocc2e4c9…/Heap.ts. Content and signature remain unchanged.docs/index.html (2)
14-17: Changelog entry for version 2.6 inserted correctly.
The new version header and bullet points accurately describe performance improvements and documentation enhancements, placed above the existing 2.5 entry.
171-172: Sidebar navigation updated to include 2.6.
The “On This Page” section now lists the new 2.6 anchor, matching the changelog addition.
ericbstie
changed the title
_moveNode method by a factor of 11x - 22x_moveNode method by a factor of 12x
ericbstie
changed the title
_moveNode method by a factor of 12x_sortNodeUp/Down methods and _moveNode method (11x improvement)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/HeapAsync.ts (3)
434-436: Consider starting heapify atfloor(n/2) - 1to skip the last leafGreat fix moving the start index from
lengthtolength / 2; that eliminates ~50 % of the useless iterations.
You can shave off the final, still-superfluous pass by subtracting one:-for (let i = Math.floor(this.heapArray.length / 2); i >= 0; --i) { +for (let i = Math.floor(this.heapArray.length / 2) - 1; i >= 0; --i) {
i = floor(n/2) - 1is the index of the last parent; anything beyond it is guaranteed to be a leaf and already satisfies the heap property.
741-760: Nice “hole” optimisation; minor micro-optimisation opportunityThe new implementation cuts heap swaps dramatically—👍.
Small tweak: you computecompare(left, right)even whenrightis out of range; you can avoid oneawaiton the common “single-child” path:-const best = - right >= length || (await this.compare(this.heapArray[left], this.heapArray[right])) < 0 - ? left - : right; +let best = left; +if (right < length && + (await this.compare(this.heapArray[left], this.heapArray[right])) > 0) { + best = right; +}This keeps logic identical while preventing an unnecessary promise allocation when the right child doesn’t exist.
767-778: Mirror optimisation in_sortNodeUp; early‐exit in tight loopThe refactor is correct and clearer; one tiny speed win: check
i !== 0before theawait, so we don’t pay the promise cost once we’ve bubbled to the root:while (i > 0) { - const pi = HeapAsync.getParentIndexOf(i); - if ((await this.compare(value, this.heapArray[pi])) < 0) { + const pi = HeapAsync.getParentIndexOf(i); + // If already in place, break early without awaiting next loop + if ((await this.compare(value, this.heapArray[pi])) < 0) {Not a huge deal, but every micro-promise counts inside a tight heap loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/heap-js.es5.jsis excluded by!**/dist/**dist/heap-js.umd.jsis excluded by!**/dist/**
📒 Files selected for processing (11)
docs/assets/main.js(1 hunks)docs/assets/style.css(1 hunks)docs/functions/toInt.html(1 hunks)docs/index.html(6 hunks)docs/modules.html(1 hunks)docs/types/AsyncComparator.html(1 hunks)docs/types/AsyncIsEqual.html(1 hunks)docs/types/Comparator.html(1 hunks)docs/types/IsEqual.html(1 hunks)src/Heap.ts(5 hunks)src/HeapAsync.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/functions/toInt.html
- docs/modules.html
- docs/index.html
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/types/IsEqual.html
- docs/types/Comparator.html
- docs/types/AsyncIsEqual.html
- docs/types/AsyncComparator.html
- src/Heap.ts
🧰 Additional context used
🪛 Biome (1.9.4)
docs/assets/main.js
[error] 4-4: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 4-4: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 4-4: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 4-4: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 4-4: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 4-4: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 4-4: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 5-5: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 6-6: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 4-4: Shouldn't redeclare 'E'. Consider to delete it or rename it.
'E' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'E'. Consider to delete it or rename it.
'E' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'E'. Consider to delete it or rename it.
'E' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'E'. Consider to delete it or rename it.
'E' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'E'. Consider to delete it or rename it.
'E' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'E'. Consider to delete it or rename it.
'E' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'E'. Consider to delete it or rename it.
'E' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'c'. Consider to delete it or rename it.
'c' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'f'. Consider to delete it or rename it.
'f' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'y'. Consider to delete it or rename it.
'y' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'f'. Consider to delete it or rename it.
'f' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'y'. Consider to delete it or rename it.
'y' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'y'. Consider to delete it or rename it.
'y' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'Q'. Consider to delete it or rename it.
'Q' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'T'. Consider to delete it or rename it.
'T' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'c'. Consider to delete it or rename it.
'c' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'd'. Consider to delete it or rename it.
'd' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 's'. Consider to delete it or rename it.
's' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'r'. Consider to delete it or rename it.
'r' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'r'. Consider to delete it or rename it.
'r' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
🔇 Additional comments (3)
docs/assets/style.css (2)
994-996: Skip: No functional impact from comment reformatting
The conversion to a multi-line, asterisk-prefixed comment is purely stylistic and doesn’t alter any behavior. No further action needed.
1001-1002: Approve: Accordion arrow now rotates for h5 headings
Extending the selector to targeth5 > svg:first-childensures closed accordions with<h5>summaries also display the rotated arrow. Well aligned with the existing patterns.docs/assets/main.js (1)
2-2: Approve: Expanded translation mappings for all code entity kinds
Thewindow.translationsobject now covers all numerickind_*identifiers from TypeDoc, matching their human-readable labels. No issues spotted.
| _moveNode(j: number, k: number): void { | ||
| [this.heapArray[j], this.heapArray[k]] = [this.heapArray[k], this.heapArray[j]]; | ||
| const temp = this.heapArray[j]; | ||
| this.heapArray[j] = this.heapArray[k]; | ||
| this.heapArray[k] = temp; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
_moveNode is now dead code – delete it to avoid maintenance overhead
After the refactor, neither _sortNodeDown nor _sortNodeUp call _moveNode, and a quick scan shows no other usages. Keeping an unused helper invites confusion and bloats the bundle.
- _moveNode(j: number, k: number): void {
- const temp = this.heapArray[j];
- this.heapArray[j] = this.heapArray[k];
- this.heapArray[k] = temp;
- }
+ // _moveNode <-- remove entire method (no callers left)Run this to confirm zero references:
🏁 Script executed:
#!/bin/bash
rg --line-number "_moveNode\(" | grep -v "function _moveNode"Length of output: 683
Tests reference _moveNode—update tests or retain the method before deleting
The _moveNode helper is exercised by your private‐methods tests, so removing it outright will break those specs. You have two options:
• Remove or refactor these tests to use the public API (e.g., swap nodes by calling _sortNodeUp/_sortNodeDown indirectly)
• Keep _moveNode (and its declarations in HeapAsync/Heap and the .d.ts files) until the tests no longer depend on it
Affected locations:
- src/HeapAsync.ts (lines 729–733)
- src/Heap.ts (around line 805)
- tests/heap/heap-private-methods.test.ts (lines 32–37)
- tests/heap-async/heap-async-private-methods.test.ts (lines 41–46)
- dist/types/HeapAsync.d.ts
- dist/types/Heap.d.ts
Committable suggestion skipped: line range outside the PR's diff.
|
The 3 improvements to the
|
ericbstie
force-pushed
the
perf/moveNode-optimization
branch
from
0789d20 to
877e37f
Compare
|
@ignlg Is this able to be merged? The tests pass locally (not sure why CI failed, but the log expired). If this merges, then this library will be competitive with the fastest implementations. And due to the clean code, good testing, and full API, it would be the preferred implementation by a large margin! |
|
This PR must point to |
3 participants
Fixes #661. Also fixes #665.
Significantly improves runtime:
More details in #661 🚀 🚀 🚀
Summary by CodeRabbit
New Features
Documentation
Refactor