scriptoriumadmin/GitNexus
1
0
Fork 0
mirror of https://github.com/abhigyanpatwari/GitNexus.git synced 2026-11-10 08:27:27 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 7

perf(scope-resolution): drop the string each membership test built per candidate

Browse source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kim7UK3kPhF1ZRZDuuvnus
This commit is contained in:
Gergo Magyar 2026-08-12 08:23:01 +00:00
parent 43bea3639d
commit 0bc8ede478
4 changed files with 167 additions and 38 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

77
gitnexus/src/core/ingestion/import-resolvers/csharp.ts
View file

@ -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<readonly number[]> {
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 `/<dirPrefix>/`, 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, '/');

54
gitnexus/src/core/ingestion/import-resolvers/go.ts
View file

@ -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 `/<pkg>/` 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 `/<pkg>/` 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]);
}

65
gitnexus/src/core/ingestion/import-resolvers/package-dir-index.ts
View file

@ -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 <packagePath>" 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<reado
const lastSegment = pkgPath.slice(pkgPath.lastIndexOf('/') + 1);
const dirs = index.dirsByLastSegment.get(lastSegment);
if (dirs === undefined) return;
const needle = `/${pkgPath}/`;
// `('/' + D + '/').endsWith('/' + P + '/')` ⟺ `D === P || D.endsWith('/' + P)`,
// which is the same predicate without the two strings per candidate the
// wrapped form built: 32.58 ns → 8.22 ns per candidate, 3.96x, over 2001
// directories of which 668 match (Node 22.18.0, best-of-80 after 300 warmup
// passes). Verified exhaustively rather than argued, over every pair of
// strings up to length 5 over `{a, b, /}` including the empty string —
// 132 496 pairs, 911 of them matching: 0 divergences. The match count is
// reported beside the timing on purpose: two predicates that agree on `false`
// everywhere also show 0 divergences.
//
// Worth the care because this loop is genuinely hot: Go's GOPATH fallback
// calls `matchingDirs` once per import-path segment over the bucket holding
// EVERY directory that shares the queried last segment (every service's
// `internal`). At 1000 services × 100 000 unresolved imports × 4 segments
// that is ~34.6 s against ~14.0 s, and ~0.5 GB of transient garbage not
// allocated.
//
// There is nothing further to win by reaching for the two-argument
// `endsWith(search, endPosition)` or for `startsWith(needle, pos)`: on the
// same data all three land together — 8.15 ns one-argument, 8.55 ns
// two-argument, 8.54 ns `startsWith` — and against the unwrapped string the
// two-argument form is character-for-character the same test. A "the 2-arg
// overload leaves V8's fast path, 20x" claim was measured during review and
// did NOT reproduce here or in two independent re-runs; its 1.87 ns baseline
// was a one-argument call whose needle failed the length/last-char precheck
// and early-exited without comparing. Recorded because the retraction is the
// useful part: compare forms that do the same work and report the hit count.
//
// The equality arm also carries the length guard the `indexOf` form needed: a
// shorter `dir` is simply false, where `indexOf` returned -1 and
// `haystack.length - needle.length` could also be -1 and report a bogus match.
const suffix = `/${pkgPath}`;
for (const dir of dirs) {
const haystack = `/${dir}/`;
// `endsWith` subsumes the length guard the `indexOf` form needed: a shorter
// haystack is simply false, where `indexOf` returned -1 and
// `haystack.length - needle.length` could also be -1 and report a bogus
// match.
if (!haystack.endsWith(needle)) continue;
if (dir !== pkgPath && !dir.endsWith(suffix)) continue;
const files = index.filesByDir.get(dir);
if (files !== undefined) yield files;
}

9
gitnexus/src/core/ingestion/import-resolvers/utils.ts
View file

@ -97,6 +97,15 @@ export interface SuffixIndex {
/**
* Get all files in a directory suffix.
*
* `dirSuffix` is matched as a SEGMENT-aligned directory suffix — every
* returned file is a direct child of a directory `D` with
* `D === dirSuffix || D.endsWith('/' + dirSuffix)`. Callers may rely on this
* and skip a direct-child re-check; `import-resolvers/csharp.ts` step 2 does
* exactly that. It bounds what may be RETURNED, not what must be found: an
* implementation is free to answer with fewer files, and the root-anchored
* index in `languages/php/import-target.ts` answers only the `D === dirSuffix`
* arm.
*
* `readonly` is the CONTRACT, and it is the contract for every implementation
* of this interface, not a description of any one of them: an implementation
* is free to return its own bucket by reference, so callers must treat the