From 0bc8ede478aeb02de57cb59114f9ef7993c516f8 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Wed, 12 Aug 2026 08:23:01 +0000 Subject: [PATCH] perf(scope-resolution): drop the string each membership test built per candidate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three `endsWith` membership tests each minted a decorated copy of the directory once per candidate, per import. The decoration cancels: ('/' + D + '/').endsWith('/' + P + '/') <=> D === P || D.endsWith('/' + P) (D + '/').endsWith(P + '/') <=> D.endsWith(P) Verified exhaustively rather than argued — every pair of strings up to length 5 over `{a, b, /}` including the empty string, 132496 pairs, 0 divergences, with the match count reported beside it because two predicates that agree on `false` everywhere also show 0 divergences. `matchingDirs` 32.58 -> 8.22 ns/candidate (3.96x), `matchingDirPositions` 64.9 -> 18.4 ns. C#'s deliberate unanchoredness survives verbatim: `src/SubModels` still answers `Models`. `resolveGoPackage` was the opposite of a win — the rewrite in this branch left the `'/' + path` cons the old `includes` guard used to short-circuit, and the first `endsWith` forces V8 to flatten it once per file. Working on the raw path with an explicit start index is 4.8x faster than that and 1.78x faster than the code before this branch. It also now reuses `resolveGoPackageDir` instead of re-deriving six of its lines. Three claims these files make are corrected while they are open: - `package-dir-index.ts` argued the rule was accidental because a sixth implementation never had it, "wired as `importResolver` by `languages/{java,kotlin}.ts`" and therefore live. It is wired and not read: `provider.importResolver` is consumed only at `import-target-adapter.ts:74-75`, and that module's exports have no importer outside their own unit test, while its docblock claims it is threaded through `finalizeScopeModel`. The argument survives on the pre-index-scan derivation; `jvm.ts` is evidence about how the predicate was written, not about live behaviour. Whether those resolvers should be deleted or wired is left as an open question. - `csharp.ts` derived the empty `dirPrefix` case from "any path whose first slash is its last", which is wrong in both directions: `src/X.cs` satisfies it and emits no empty key, `a//X.cs` violates it and does. The conclusion stands and the filter stays — it is what rejects `a//X.cs`. - Step 2 returns on its first push, so widening it also suppresses step 3's unanchored leg. The narrower answer is the more precise one, but it was an unstated output change. `SuffixIndex.getFilesInDir` now states the segment-alignment its callers rely on, bounded as a guarantee about what may be RETURNED — php's root-anchored index answers only the equality arm. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Kim7UK3kPhF1ZRZDuuvnus --- .../core/ingestion/import-resolvers/csharp.ts | 77 ++++++++++++++++--- .../src/core/ingestion/import-resolvers/go.ts | 54 +++++++++---- .../import-resolvers/package-dir-index.ts | 65 +++++++++++++--- .../core/ingestion/import-resolvers/utils.ts | 9 +++ 4 files changed, 167 insertions(+), 38 deletions(-) diff --git a/gitnexus/src/core/ingestion/import-resolvers/csharp.ts b/gitnexus/src/core/ingestion/import-resolvers/csharp.ts index 6886118cd..e8d6fdf7a 100644 --- a/gitnexus/src/core/ingestion/import-resolvers/csharp.ts +++ b/gitnexus/src/core/ingestion/import-resolvers/csharp.ts @@ -39,7 +39,17 @@ import { csharpSuffixFallbackAllowed } from '../csharp-namespace-gate.js'; * have to move together — see the note at step 3. * - the needle ends with '/', so every occurrence of it lies wholly inside * `D + '/'` and never reaches into the file name — which is what lets the - * whole test be evaluated on `D` alone. + * whole test be evaluated on `D` alone; + * - and then the '/' cancels. `(D + '/').endsWith(P + '/')` IS `D.endsWith(P)`: + * the appended character only ever matches itself, so it decides nothing and + * the comparison of everything before it is unchanged. The predicate the code + * actually runs is therefore + * + * match ⟺ D.endsWith(dirPrefix) + * + * with no concatenation on either side. Verified rather than argued: over + * every ordered pair of strings up to length 5 over `{a, b, '/'}` including + * the empty string — 132 496 pairs — the two forms disagreed 0 times. * * NOT the same query as `package-dir-index.ts`, and the difference is exactly * one character on each side: that module tests `'/'+D+'/'` against @@ -52,6 +62,11 @@ import { csharpSuffixFallbackAllowed } from '../csharp-namespace-gate.js'; * "cleaned up" into a reuse of `filesDirectlyInPkgDir` — see * `test/unit/import-resolvers/csharp-csproj-parity.test.ts`. * + * That one character is also why the cancellation above empties this predicate + * out but not that one: the decoration is one term per side here (`D + '/'`) and + * two there (`'/' + D + '/'`), and only the TRAILING '/' cancels. Here nothing + * is left to concatenate; there the leading segment anchor has to stay. + * * Candidates are narrowed by the directory's LAST segment, the same * O(directories) bucket `package-dir-index.ts` uses instead of an * O(files × depth) suffix map (#2649). @@ -178,15 +193,32 @@ function* matchingDirPositions( index: CsharpNamespaceDirIndex, dirPrefix: string, ): Generator { - const needle = dirPrefix + '/'; for (const dir of candidateDirs(index, dirPrefix)) { - const haystack = dir + '/'; + // `(dir + '/').endsWith(dirPrefix + '/')` IS `dir.endsWith(dirPrefix)` — the + // appended '/' only ever matches itself, so it decides nothing and BOTH + // concatenations go. Exhaustively verified, not assumed: 0 disagreements + // over every ordered pair of strings up to length 5 over `{a, b, '/'}` + // including '' (132 496 pairs). Measured 64.9 ns -> 18.4 ns per candidate + // (Node 22.18); the `dir + '/'` was paid once per candidate, on every sweep + // of the last-segment keys. + // + // Still deliberately UNANCHORED (no leading '/'), so `src/SubModels` keeps + // answering `Models` — see the derivation above. That is also exactly why + // the reduction empties this predicate out while `package-dir-index.ts` + // keeps its concatenations: one decorating term per side here, two there, + // and only the trailing one cancels. + // // `endsWith` subsumes the length guard the `indexOf` form needed: a shorter - // haystack is simply false, where `indexOf` returned -1 and + // `dir` is simply false, where `indexOf` returned -1 and // `haystack.length - needle.length` could also be -1 and report a bogus - // match. Still deliberately UNANCHORED (no leading '/'), so `src/SubModels` - // keeps answering `Models` — see the derivation above. - if (!haystack.endsWith(needle)) continue; + // match. + // + // Do NOT "finish the job" with the two-argument overload. `endsWith(search, + // endPosition)` measured 8.8-11.8 ns against 9.5-14.9 ns for the + // one-argument form across seven call-site shapes (Node 22.18) — a wash — + // and `dir.endsWith(dirPrefix, dir.length)` is character-for-character this + // same test anyway. There is nothing left here to win. + if (!dir.endsWith(dirPrefix)) continue; const positions = index.positionsByDir.get(dir); if (positions !== undefined) yield positions; } @@ -295,12 +327,33 @@ export function resolveCSharpImportInternal( // pairs. It rejected before #2881 only because it asked `indexOf` for the // FIRST `//`, which is the rule that issue removed. // + // That widening does not stay inside step 2's own bucket. This step + // returns as soon as it pushes anything, so a query it used to answer with + // nothing now also SUPPRESSES step 3, whose unanchored match set is a + // strict superset: over `SubModels/Models/F1.cs` + `SubModels/F3.cs`, + // `using App.Models` answered both through step 3 and now answers only the + // first through step 2. The new answer is the more precise one — a + // directory literally named `Models` beating a character-suffix hit on + // `SubModels` — and it is what this module's step-2-before-step-3 layering + // asks for, so it is kept rather than worked around. Pinned absolutely by + // the parity test, which is differentially blind to it (its frozen legacy + // copy moved in lockstep with this line). + // // The empty prefix is the exception and keeps a real filter. `getDirMap` - // emits an empty directory suffix for any path whose first slash is its - // last — including a doubled slash, `a//X.cs` — so the bucket is not - // "one directory deep" on its own, and step 3 answers that same query - // from `singleSegmentDirs`, which is. Filtering on `D` holding no slash - // is what keeps steps 2 and 3 in agreement. + // keys a file under every suffix of its DIRECTORY, so it emits the EMPTY + // one exactly when that directory's last component is empty: a leading '/' + // on a root-level file, or a doubled slash immediately before the file + // name. Probed against `getDirMap`'s own key emission: + // + // src/X.cs -> ['src:.cs'] no empty key + // /X.cs -> [':.cs'] empty key + // a//X.cs -> [':.cs', 'a/:.cs'] empty key + // /a/b/X.cs -> ['b:.cs', 'a/b:.cs', '/a/b:.cs'] no empty key + // + // So the `''` bucket is not "one directory deep" on its own — `a//X.cs` + // sits in it two components down — while step 3 answers that same query + // from `singleSegmentDirs`, which is. Filtering on `D` holding no slash is + // what rejects `a//X.cs` and keeps steps 2 and 3 in agreement. for (const f of dirFiles) { if (dirPrefix === '') { const normalized = f.replace(/\\/g, '/'); diff --git a/gitnexus/src/core/ingestion/import-resolvers/go.ts b/gitnexus/src/core/ingestion/import-resolvers/go.ts index d4b36e145..eb87b0997 100644 --- a/gitnexus/src/core/ingestion/import-resolvers/go.ts +++ b/gitnexus/src/core/ingestion/import-resolvers/go.ts @@ -3,10 +3,23 @@ * * Strategy lives in configs/go.ts. * This file contains the shared helpers used by the strategy. + * + * **Reachability, as of #2929:** nothing in production calls either export + * today. The only path in is `configs/go.ts` → `createImportResolver` → + * the `importResolver` field on Go's `LanguageProvider`, and that field is + * read at exactly two lines — `import-target-adapter.ts:74-75` — whose two + * exports (`buildImportTargetWorkspace`, + * `resolveImportTargetAcrossLanguages`) have no importer anywhere but their + * own unit test. So this is a live-looking but currently unwired leg; the + * tests in `test/unit/import-resolvers/go-package-resolve.test.ts` are the + * only thing watching it. */ import type { GoModuleConfig } from '../language-config.js'; +/** `'/'`, for the parent-directory boundary check in `resolveGoPackage`. */ +const SLASH_CODE = 47; + /** * Extract the package directory suffix from a Go import path. * Returns the suffix string (e.g., "/internal/auth/") or null if invalid. @@ -28,27 +41,38 @@ export function resolveGoPackage( normalizedFileList: readonly string[], allFileList: readonly string[], ): string[] { - if (!importPath.startsWith(goModule.modulePath)) return []; + // Identical to the six lines this used to re-derive; `resolveGoPackageDir` + // returns the '/'-wrapped form and the scan wants the bare path, so unwrap. + const pkgDir = resolveGoPackageDir(importPath, goModule); + if (pkgDir === null) return []; + const relativePkg = pkgDir.slice(1, -1); // "/internal/auth/" → "internal/auth" - // Strip module path to get relative package path - const relativePkg = importPath.slice(goModule.modulePath.length + 1); // e.g., "internal/auth" - if (!relativePkg) return []; - - const pkgSuffix = '/' + relativePkg + '/'; + const pkgLen = relativePkg.length; // >= 1: `resolveGoPackageDir` rejects empty const matches: string[] = []; for (let i = 0; i < normalizedFileList.length; i++) { - // Prepend '/' so paths like "internal/auth/service.go" match suffix "/internal/auth/" - const normalized = '/' + normalizedFileList[i]; + const normalized = normalizedFileList[i]; if (!normalized.endsWith('.go') || normalized.endsWith('_test.go')) continue; // The file's PARENT directory ends with the package path — the same - // predicate `package-dir-index.ts` states, in the same `endsWith` form. - // This used to ask `indexOf` for the FIRST `//` and then check that - // nothing after it held a slash, which made `a/pkg/b/pkg/x.go` not a member - // of `pkg` (#2881). Writing it as ends-with rather than swapping in - // `lastIndexOf` also drops the second scan the `includes` guard needed. - const lastSlash = normalized.lastIndexOf('/'); // >= 0: `normalized` starts with '/' - if (!normalized.slice(0, lastSlash + 1).endsWith(pkgSuffix)) continue; + // predicate `package-dir-index.ts` states. This used to ask `indexOf` for + // the FIRST `//` and then check that nothing after it held a slash, + // which made `a/pkg/b/pkg/x.go` not a member of `pkg` (#2881). + // + // Expressed as "`relativePkg` sits immediately before the last slash, on a + // segment boundary". The boundary is either the start of the path (an + // import matching from index 0, `internal/auth/x.go`) or a `/` — which is + // what the old `'/' + path` cons bought, at the price of a per-file + // concatenation the first `endsWith` forced V8 to flatten (#2929). + // + // Rewriting this as `endsWith(relativePkg, lastSlash)` buys nothing: the + // two-argument overload measured a wash against `startsWith(needle, pos)` + // here (10.28 ns vs 9.82 ns), so it trades the clarity of an explicit start + // index for no gain. A "the 2-arg overload leaves V8's fast path, 20x" + // claim from review did not reproduce on Node 22.18 — its baseline was a + // one-argument call that early-exited on the length precheck. + const start = normalized.lastIndexOf('/') - pkgLen; // < 0 when there is no parent dir + if (start < 0 || !normalized.startsWith(relativePkg, start)) continue; + if (start > 0 && normalized.charCodeAt(start - 1) !== SLASH_CODE) continue; matches.push(allFileList[i]); } diff --git a/gitnexus/src/core/ingestion/import-resolvers/package-dir-index.ts b/gitnexus/src/core/ingestion/import-resolvers/package-dir-index.ts index 51c389021..cb7e77ed0 100644 --- a/gitnexus/src/core/ingestion/import-resolvers/package-dir-index.ts +++ b/gitnexus/src/core/ingestion/import-resolvers/package-dir-index.ts @@ -33,10 +33,27 @@ * The strongest evidence that the rule was accidental is that a sixth * implementation of the same question never had it. `import-resolvers/jvm.ts` * answers "files directly inside a directory ending with " for - * Java and Kotlin wildcard imports, and has used `lastIndexOf` since #488 — it - * is live, wired as `importResolver` by `languages/{java,kotlin}.ts`. So before - * #2881 the Java and Kotlin LanguageProvider hook and their ScopeResolver hook - * disagreed about which files a package holds. Now all six agree. + * Java and Kotlin wildcard imports, and has used `lastIndexOf` since #488. + * + * That is evidence about how the predicate was WRITTEN, not about live + * behaviour, and the distinction matters enough to spell out. `jvm.ts` is + * reached only through `provider.importResolver`, which `languages/java.ts` and + * `languages/kotlin.ts` do wire — but that field currently has no production + * READER. Its only reader anywhere is `import-target-adapter.ts`, whose own + * docblock says it is "threaded through `finalizeScopeModel`"; nothing threads + * it, and neither that module nor its two exports + * (`buildImportTargetWorkspace`, `resolveImportTargetAcrossLanguages`) is + * referenced outside its own unit test. So `jvm.ts`'s `resolveJvmWildcard` and + * `import-resolvers/go.ts`'s `resolveGoPackage` are dormant, while THIS index, + * `csharp.ts`'s `resolveCSharpImportInternal` and Kotlin's `dirChildren` are + * the ones that run. Whether those two dormant resolvers should be deleted or + * actually wired up is an open question and wants its own issue; it is not + * settled here. + * + * The argument survives that correction intact, because it never needed the + * resolvers to be live: an independent implementation of the same question, + * written without reference to the pre-index scan, reached for `lastIndexOf`. + * The extra clause was never a rule anyone chose. All six spellings now agree. * * The length guard the `indexOf` form needed is gone with it: `endsWith` is * false for a shorter `D` instead of comparing -1 to -1. @@ -139,14 +156,40 @@ function* matchingDirs(index: PackageDirIndex, pkgPath: string): Generator