⚡ Bolt: Fast Unique Tracking via O(1) Array Buffers#407
Open
AhmmedSamier wants to merge 1 commit intomasterfrom
Open
⚡ Bolt: Fast Unique Tracking via O(1) Array Buffers#407AhmmedSamier wants to merge 1 commit intomasterfrom
AhmmedSamier wants to merge 1 commit intomasterfrom
Conversation
Replaces per-call `new Set<number>()` allocations in hot search paths (`performUnifiedSearch`) with reusable instance-level `Uint8Array` buffers and dirty-tracking lists. This provides O(1) membership checking and O(K) cleanup without instantiating new Map/Set structures, heavily reducing garbage collection pressure. Included a `try...finally` safeguard to ensure state hygiene on unexpected errors and removed writes to the tracker in the final pass. Co-authored-by: AhmmedSamier <17784876+AhmmedSamier@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
📝 WalkthroughWalkthroughThis PR replaces Set-based visited-index tracking in the search engine with reusable Uint8Array buffers to eliminate allocation overhead. A cleanup helper and try...finally safeguard prevent stale state after errors. Documentation codifies the pattern. ChangesAllocation-Free Visited Index Tracking
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 docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@language-server/src/core/search-engine.ts`:
- Around line 2375-2409: The shared visited tracker (visitedIndicesBuffer /
visitedIndicesList) is mutated by processItem which can synchronously re-enter
search; to prevent corrupting outer state, add a re-entrancy fallback: introduce
a per-instance boolean flag (e.g., this._isSearching) and in the search loop
decide whether to use the shared buffers or allocate local ones—if
this._isSearching is true, create a fresh Uint8Array and fresh list for
searchedIndices/visitedIndicesList for this invocation; otherwise set
this._isSearching = true, use the shared this.visitedIndicesBuffer /
this.visitedIndicesList, and in the try/finally ensure you clear only the
modified slots and reset this._isSearching = false; reference symbols to change:
processItem, visitedIndicesBuffer, visitedIndicesList, searchedIndices, and add
this._isSearching to guard re-entrant calls.
🪄 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: a62895cd-ede6-4b77-9586-efae7b1abe20
📒 Files selected for processing (2)
.jules/bolt.mdlanguage-server/src/core/search-engine.ts
Comment on lines
+2375
to
2409
| if (this.visitedIndicesBuffer.length < this.items.length) { | ||
| this.visitedIndicesBuffer = new Uint8Array(this.items.length); | ||
| } | ||
| this.visitedIndicesList.length = 0; | ||
|
|
||
| const searchedIndices = this.visitedIndicesBuffer; | ||
| const priorityScopesLength = priorityScopes.length; | ||
|
|
||
| for (let s = 0; s < priorityScopesLength; s++) { | ||
| const indices = this.scopedIndices.get(priorityScopes[s]); | ||
| if (indices) { | ||
| const len = indices.length; | ||
| for (let j = 0; j < len; j++) { | ||
| searchedIndices[indices[j]] = 1; | ||
| const idx = indices[j]; | ||
| searchedIndices[idx] = 1; | ||
| this.visitedIndicesList.push(idx); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (let i = 0; i < this.items.length; i++) { | ||
| if (results.length >= maxResults || token?.isCancellationRequested) break; | ||
| if (searchedIndices[i] === 0) { | ||
| processItem(i); | ||
| try { | ||
| for (let i = 0; i < this.items.length; i++) { | ||
| if (results.length >= maxResults || token?.isCancellationRequested) break; | ||
| if (searchedIndices[i] === 0) { | ||
| processItem(i); | ||
| } | ||
| } | ||
| } finally { | ||
| // Fast cleanup: clear only the modified slots using the tracked list | ||
| const modifiedLen = this.visitedIndicesList.length; | ||
| for (let i = 0; i < modifiedLen; i++) { | ||
| searchedIndices[this.visitedIndicesList[i]] = 0; | ||
| } | ||
| this.visitedIndicesList.length = 0; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard shared visited tracker against re-entrant callbacks.
processItem(i) can synchronously trigger onResult, and consumer code can re-enter search on the same SearchEngine instance. Because this path mutates shared visitedIndicesBuffer/visitedIndicesList, nested calls can reset/corrupt the outer pass state.
Suggested fix approach (keep fast path, add safe fallback only when re-entrant)
private searchRemainingItems(
priorityScopes: SearchScope[],
maxResults: number,
processItem: (i: number) => void,
results: SearchResult[],
token?: CancellationToken,
): void {
+ // Re-entrancy guard: callbacks can synchronously trigger nested searches.
+ if (this.visitedIndicesList.length > 0) {
+ const searchedIndices = new Set<number>();
+ for (let s = 0; s < priorityScopes.length; s++) {
+ const indices = this.scopedIndices.get(priorityScopes[s]);
+ if (!indices) continue;
+ for (let j = 0; j < indices.length; j++) {
+ searchedIndices.add(indices[j]);
+ }
+ }
+ for (let i = 0; i < this.items.length; i++) {
+ if (results.length >= maxResults || token?.isCancellationRequested) break;
+ if (!searchedIndices.has(i)) {
+ processItem(i);
+ }
+ }
+ return;
+ }
+
if (this.visitedIndicesBuffer.length < this.items.length) {
this.visitedIndicesBuffer = new Uint8Array(this.items.length);
}
this.visitedIndicesList.length = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (this.visitedIndicesBuffer.length < this.items.length) { | |
| this.visitedIndicesBuffer = new Uint8Array(this.items.length); | |
| } | |
| this.visitedIndicesList.length = 0; | |
| const searchedIndices = this.visitedIndicesBuffer; | |
| const priorityScopesLength = priorityScopes.length; | |
| for (let s = 0; s < priorityScopesLength; s++) { | |
| const indices = this.scopedIndices.get(priorityScopes[s]); | |
| if (indices) { | |
| const len = indices.length; | |
| for (let j = 0; j < len; j++) { | |
| searchedIndices[indices[j]] = 1; | |
| const idx = indices[j]; | |
| searchedIndices[idx] = 1; | |
| this.visitedIndicesList.push(idx); | |
| } | |
| } | |
| } | |
| for (let i = 0; i < this.items.length; i++) { | |
| if (results.length >= maxResults || token?.isCancellationRequested) break; | |
| if (searchedIndices[i] === 0) { | |
| processItem(i); | |
| try { | |
| for (let i = 0; i < this.items.length; i++) { | |
| if (results.length >= maxResults || token?.isCancellationRequested) break; | |
| if (searchedIndices[i] === 0) { | |
| processItem(i); | |
| } | |
| } | |
| } finally { | |
| // Fast cleanup: clear only the modified slots using the tracked list | |
| const modifiedLen = this.visitedIndicesList.length; | |
| for (let i = 0; i < modifiedLen; i++) { | |
| searchedIndices[this.visitedIndicesList[i]] = 0; | |
| } | |
| this.visitedIndicesList.length = 0; | |
| } | |
| // Re-entrancy guard: callbacks can synchronously trigger nested searches. | |
| if (this.visitedIndicesList.length > 0) { | |
| const searchedIndices = new Set<number>(); | |
| for (let s = 0; s < priorityScopes.length; s++) { | |
| const indices = this.scopedIndices.get(priorityScopes[s]); | |
| if (!indices) continue; | |
| for (let j = 0; j < indices.length; j++) { | |
| searchedIndices.add(indices[j]); | |
| } | |
| } | |
| for (let i = 0; i < this.items.length; i++) { | |
| if (results.length >= maxResults || token?.isCancellationRequested) break; | |
| if (!searchedIndices.has(i)) { | |
| processItem(i); | |
| } | |
| } | |
| return; | |
| } | |
| if (this.visitedIndicesBuffer.length < this.items.length) { | |
| this.visitedIndicesBuffer = new Uint8Array(this.items.length); | |
| } | |
| this.visitedIndicesList.length = 0; | |
| const searchedIndices = this.visitedIndicesBuffer; | |
| const priorityScopesLength = priorityScopes.length; | |
| for (let s = 0; s < priorityScopesLength; s++) { | |
| const indices = this.scopedIndices.get(priorityScopes[s]); | |
| if (indices) { | |
| const len = indices.length; | |
| for (let j = 0; j < len; j++) { | |
| const idx = indices[j]; | |
| searchedIndices[idx] = 1; | |
| this.visitedIndicesList.push(idx); | |
| } | |
| } | |
| } | |
| try { | |
| for (let i = 0; i < this.items.length; i++) { | |
| if (results.length >= maxResults || token?.isCancellationRequested) break; | |
| if (searchedIndices[i] === 0) { | |
| processItem(i); | |
| } | |
| } | |
| } finally { | |
| // Fast cleanup: clear only the modified slots using the tracked list | |
| const modifiedLen = this.visitedIndicesList.length; | |
| for (let i = 0; i < modifiedLen; i++) { | |
| searchedIndices[this.visitedIndicesList[i]] = 0; | |
| } | |
| this.visitedIndicesList.length = 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@language-server/src/core/search-engine.ts` around lines 2375 - 2409, The
shared visited tracker (visitedIndicesBuffer / visitedIndicesList) is mutated by
processItem which can synchronously re-enter search; to prevent corrupting outer
state, add a re-entrancy fallback: introduce a per-instance boolean flag (e.g.,
this._isSearching) and in the search loop decide whether to use the shared
buffers or allocate local ones—if this._isSearching is true, create a fresh
Uint8Array and fresh list for searchedIndices/visitedIndicesList for this
invocation; otherwise set this._isSearching = true, use the shared
this.visitedIndicesBuffer / this.visitedIndicesList, and in the try/finally
ensure you clear only the modified slots and reset this._isSearching = false;
reference symbols to change: processItem, visitedIndicesBuffer,
visitedIndicesList, searchedIndices, and add this._isSearching to guard
re-entrant calls.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 What
Replaced
Set<number>allocations insearchRemainingItemsandperformUnifiedSearchtracking passes with reusableUint8Arrayandnumber[]array trackers.🎯 Why
$O(1)$ flat array index access. It was causing unnecessary garbage collection thrashing and blocking the thread in huge workspaces.
performUnifiedSearchis executed frequently and on large scale sets. Allocating new Sets and calling.add()/.has()scales poorly in JavaScript compared to direct📊 Impact
new Set()object allocations per search request.searchAllItemspass.🔬 Measurement
Review
language-server/src/core/search-engine.tslines 1642 to seevisitedIndicesBufferandvisitedIndicesListacting as persistent tracking instances. A simple benchmark measuring memory overhead will demonstrate significantly lower GC latency over repeated bursts of text-box typing.PR created automatically by Jules for task 11238349885308795904 started by @AhmmedSamier
Summary by CodeRabbit