-
Notifications
You must be signed in to change notification settings - Fork 10
perf(globs): memoize compiled include/exclude globs per build #1005
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
bc81ce7
perf(globs): memoize compiled include/exclude globs per build
carlos-alm e83741d
fix(globs): use JSON.stringify for unambiguous cache keys (#1005)
carlos-alm 8922af4
fix(globs): use VecDeque for O(1) FIFO eviction (#1005)
carlos-alm 8435039
Merge branch 'main' into fix/memoize-globs
carlos-alm f0533db
Merge branch 'main' into fix/memoize-globs
carlos-alm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| /** | ||
| * Unit tests for src/shared/globs.ts — covers the memoization added so | ||
| * long-running hosts (watch mode, MCP server) don't recompile include/exclude | ||
| * globs on every buildGraph call. | ||
| */ | ||
| import { afterEach, describe, expect, it } from 'vitest'; | ||
| import { clearGlobCache, compileGlobs } from '../../src/shared/globs.js'; | ||
|
|
||
| describe('compileGlobs', () => { | ||
| afterEach(() => { | ||
| clearGlobCache(); | ||
| }); | ||
|
|
||
| it('returns an empty list for undefined or empty input', () => { | ||
| expect(compileGlobs(undefined)).toHaveLength(0); | ||
| expect(compileGlobs([])).toHaveLength(0); | ||
| }); | ||
|
|
||
| it('compiles each pattern to a RegExp anchored with ^…$', () => { | ||
| const [inc, exc] = [compileGlobs(['src/**/*.ts']), compileGlobs(['**/*.test.ts'])]; | ||
| expect(inc).toHaveLength(1); | ||
| expect(exc).toHaveLength(1); | ||
| expect(inc[0]!.test('src/foo/bar.ts')).toBe(true); | ||
| expect(inc[0]!.test('tests/foo.ts')).toBe(false); | ||
| expect(exc[0]!.test('src/foo/bar.test.ts')).toBe(true); | ||
| expect(exc[0]!.test('src/foo/bar.ts')).toBe(false); | ||
| }); | ||
|
|
||
| it('reuses the same array for identical pattern lists (memoization)', () => { | ||
| const first = compileGlobs(['src/**/*.ts', '**/*.test.ts']); | ||
| const second = compileGlobs(['src/**/*.ts', '**/*.test.ts']); | ||
| // Repeated calls with equivalent content return the exact same array — this is | ||
| // what makes repeated buildGraph invocations in a long-running host cheap. | ||
| expect(second).toBe(first); | ||
| }); | ||
|
|
||
| it('treats different pattern lists as distinct cache entries', () => { | ||
| const a = compileGlobs(['src/**/*.ts']); | ||
| const b = compileGlobs(['src/**/*.js']); | ||
| expect(a).not.toBe(b); | ||
| expect(a[0]!.test('src/x.ts')).toBe(true); | ||
| expect(b[0]!.test('src/x.js')).toBe(true); | ||
| expect(a[0]!.test('src/x.js')).toBe(false); | ||
| }); | ||
|
|
||
| it('treats different pattern orderings as distinct cache entries', () => { | ||
| // Order matters for glob matching semantics (even if results are the same | ||
| // set here, callers may rely on iteration order elsewhere). Two lists with | ||
| // the same patterns in different orders get independent cache slots. | ||
| const a = compileGlobs(['a/**', 'b/**']); | ||
| const b = compileGlobs(['b/**', 'a/**']); | ||
| expect(a).not.toBe(b); | ||
| }); | ||
|
|
||
| it('returns a frozen array to prevent accidental mutation of cached values', () => { | ||
| const patterns = compileGlobs(['src/**/*.ts']); | ||
| expect(Object.isFrozen(patterns)).toBe(true); | ||
| }); | ||
|
|
||
| it('clearGlobCache drops memoized entries', () => { | ||
| const before = compileGlobs(['src/**/*.ts']); | ||
| clearGlobCache(); | ||
| const after = compileGlobs(['src/**/*.ts']); | ||
| // Same content, but cleared cache forces a fresh compilation → different | ||
| // array identity. Test guards against silent cache leaks across suites. | ||
| expect(after).not.toBe(before); | ||
| }); | ||
| }); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vec::remove(0)is O(n)self.order.remove(0)shifts every remaining element left on each eviction. At cap=32 the cost is negligible, but aVecDequewould give O(1)pop_frontat no extra complexity and makes the intent clearer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8922af4 — switched
orderfromVec<Vec<String>>toVecDeque<Vec<String>>and replacedremove(0)/pushwithpop_front/push_back. O(1) eviction and the intent reads more clearly.cargo test -p codegraph-core file_collectorstill passes all 8 tests (including the 2 memoization tests).