Skip to content

perf(impact): avoid per-candidate array allocation#51

Merged
PatrickSys merged 1 commit intomasterfrom
perf/impact-targetset-loop
Feb 28, 2026
Merged

perf(impact): avoid per-candidate array allocation#51
PatrickSys merged 1 commit intomasterfrom
perf/impact-targetset-loop

Conversation

@PatrickSys
Copy link
Owner

Addresses PR #50 inline review comment (discussion_r2867598693): avoid allocating Array.from(targetSet) on each addCandidate call by iterating the set directly. No behavior change.

@greptile-apps
Copy link

greptile-apps bot commented Feb 28, 2026

Greptile Summary

This PR is a focused micro-optimization inside the impact-analysis addCandidate closure in src/tools/search-codebase.ts. It replaces Array.from(targetSet).some(...) with a direct for...of iteration over the Set, eliminating a transient array allocation that previously occurred on every invocation of addCandidate.

  • The early-return semantics are preserved exactly: the return inside the for...of loop mirrors the Array.from(...).some(...) short-circuit.
  • No behavior change — both paths iterate targetSet values and bail out immediately on the first pathsMatch hit.
  • The optimization is most meaningful when addCandidate is called at high frequency (large import graphs with many hop-1/hop-2 candidates).

Confidence Score: 5/5

  • This PR is safe to merge — it is a correct, behaviour-preserving micro-optimisation with no risk.
  • The change is a single logical unit (3 lines), the semantics of the original .some() call are perfectly replicated by the for...of with early return, and there are no new dependencies or side-effects introduced.
  • No files require special attention.

Important Files Changed

Filename Overview
src/tools/search-codebase.ts Replaces Array.from(targetSet).some(...) with a direct for...of loop over the Set to eliminate per-call array allocation inside addCandidate; behavior is identical and the change is correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["addCandidate(file, hop, line)"] --> B{"for each t in targetSet"}
    B -- "pathsMatch(t, file)" --> C["return (skip — file is a target)"]
    B -- "no match, set exhausted" --> D{"candidates.get(file) exists?"}
    D -- "yes, existing.hop ≤ hop" --> E["return (existing entry is better/equal)"]
    D -- "no / existing hop is worse" --> F["candidates.set(file, {file, hop, line})"]
Loading

Last reviewed commit: faf6e73

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@PatrickSys PatrickSys merged commit 04e68eb into master Feb 28, 2026
3 checks passed
@PatrickSys PatrickSys deleted the perf/impact-targetset-loop branch February 28, 2026 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant