Stop re-parsing and re-mirroring the project on every type-aware parse - #246
Draft
wagenet wants to merge 4 commits into
Draft
Stop re-parsing and re-mirroring the project on every type-aware parse#246wagenet wants to merge 4 commits into
wagenet wants to merge 4 commits into
Conversation
The patched ts.sys.readFile runs replaceExtensions over every .ts source TypeScript pulls into the program, and replaceExtensions does a full createSourceFile to look for .gts module specifiers. Ember apps import components extensionless, so that parse is thrown away for nearly every file, once per file, while the program is being built. Only .gts is ever rewritten, and every specifier form that reaches the rewrite carries a literal .gts in the source text: spellings that hide it behind an escape sequence or a line continuation already fail the length assertion and leave the file alone. A substring miss therefore means the walk cannot change anything. Over 400 app-sized .ts sources this drops from 38.6ms to 105us. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
syncMtsGtsSourceFiles runs after every type-aware parse and walks the whole program, including lib.d.ts and every .d.ts reachable from node_modules. Two costs there scaled with project size rather than with the file being linted. Per source file, syncVirtualFile ran twice and built up to four RegExps per call before concluding the file was an ordinary .ts it did not care about. A single endsWith now drops the uninteresting majority, and the two surviving suffix patterns are module constants. The branches are mutually exclusive: no path ends in both .gts and .mts, and only files linked here ever carry a virtual flag. Per .gts, mirroring a source onto its twin copies ~50 properties with Object.assign, and it ran for every .gts in the program on every parse. TypeScript hands back the same SourceFile object until a file's content changes, at which point it builds a new one with a new version, so a WeakMap from twin to source plus a version check identifies the copies already in place. Instrumenting a full lint of a synthetic project found 89,999 of 90,000 mirror operations re-copying an unchanged source, with only lineMap and modifierFlagsCache ever differing on the twin; both are derived lazily from text, which is identical on both sides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The project-mode scenarios all measure warm parses, which happen after the program is built, so none of them reach the patched ts.sys.readFile and its per-file cost was invisible to the bench. Sources are generated for this scenario rather than reused from the project above: the project's helper modules are a few lines each, and this cost tracks file size. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
🏎️ Benchmark ComparisonParse
Full mitata output
Full mitata output |
The comment on the replaceExtensions pre-check claimed the escaped- specifier case was already a no-op. It is not: gjs-gts-parser calls replaceExtensions outside the readFile try/catch, so `bad replacement` reached ESLint as a parsing error on an otherwise valid file. Skipping the walk swallows that throw. Say so, and cover it with a test. The memoization comment stated object-identity-plus-version as a general TypeScript property. It holds for the hosts used in type-aware linting; user-supplied `programs` never set a version, and there identity carries the guard alone. None of the new tests failed when the memo guard was deleted, because Object.assign does not clear properties absent from the source. Pin it with one that the copy would overwrite. Fold the replaceExtensions cases into the block that already covers that function in parser.test.js rather than opening a second describe of the same name, and cover the gaps around the .gjs fallback, .mjs orphan invalidation, and the virtual flag landing on a .ts fallback twin. Bench a source that does contain a .gts alongside the one that does not, so the pair brackets the change instead of only measuring the case it was built to win, and skip the comparison when the control predates the replaceExtensions export. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wagenet
force-pushed
the
wagenet/eep-replaceextensions-perf
branch
from
August 14, 2026 20:45
7bf1672 to
3be7061
Compare
NullVoxPopuli
marked this pull request as draft
August 14, 2026 21:44
Member
|
converting to draft, because it appears things are still in flux |
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
2 participants
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.
Type-aware linting of a large Ember app spends more time in this parser than I expected. Two places in
src/parser/ts-patch.jsdo work proportional to the size of the whole project, on inputs where the work can't change the result.One deliberate behavior change falls out of this, on a pathological input. It's called out under §1 rather than buried.
1.
replaceExtensionsparses every.tsfile looking for imports that aren't therepatchTs()wrapsts.sys.readFile, so every.tssource TypeScript pulls into the program goes throughreplaceExtensions, which runs a fullts.createSourceFileto look for.gtsmodule specifiers.In an Ember app basically no
.tsfile names a.gts; components get imported extensionless. The parse is thrown away for nearly every file, once per file, while the program is being built.The function only rewrites
.gts(.gjsspecifiers are left alone today, and that's unchanged here), and every specifier the walk can successfully rewrite carries a literal.gtsin the source text. So:The one behavior change. A specifier that spells the extension behind an escape or a line continuation has no literal
.gtsin its raw text, so the pre-check skips a file the walk used to reach:The walk never rewrote those either. It replaces the raw span with the cooked value, so the file came out shorter and tripped the
length !== jsCode.lengthassertion. I'd assumed that throw was harmless becausereadFilecatches it, butgjs-gts-parser.jscallsreplaceExtensionsoutside that try/catch, so it actually surfaced asParsing error: bad replacementon a file that is valid TypeScript. Confirmed onmain:I think that's an improvement, but it is a change, and it's tested rather than assumed.
2.
syncMtsGtsSourceFilesre-mirrors the whole project on every parseThis runs after every type-aware parse and walks every file in the program, including
lib.d.tsand every.d.tsreachable fromnode_modules. Two costs in there.The per-file one:
syncVirtualFileran twice per source file and built up to fournew RegExp(...)per call before working out that the file was an ordinary.tsit didn't care about. Now a singleendsWithdrops the uninteresting majority, and the two surviving suffix patterns are module constants.The per-
.gtsone is bigger: mirroring a source onto its virtual twin copies about 50 properties withObject.assign, and it ran for every.gtsin the program on every parse. An app with 8,000.gtsfiles paid 8,000 of those per linted file. The hosts used for type-aware linting hand back the sameSourceFileobject until a file's content changes and build a new one when it does, so aWeakMapfrom twin to source plus a version check identifies the copies already in place.I instrumented that before touching it rather than assuming it was safe to skip. Over a full lint of a synthetic project, 89,999 of 90,000 mirror operations were re-copying an unchanged source file. The only values that ever differed on the twin were
lineMapandmodifierFlagsCache, both derived lazily fromtext, which is shared by reference.Two things reviewers should know about the guard:
options.programspath the program comes from a plaints.createProgram, which never setsversion. There the version clause isundefined === undefinedand object identity carries the guard by itself. That's sound as long as a user-supplied program doesn't mutate itsSourceFiles in place, which none does, but it's weaker than it looks.idon every pass, which aliased the.gtsand.mtsfiles into onenodeLinksslot in the checker from the second sync onward. They now keep separate ids. I believe that's the correct behavior rather than a regression, and the differential check below finds no observable difference, but it is a change in what the checker sees.Numbers
pnpm bench:project:compareagainstmainwithBENCH_PROJECT_FILES=400(400.gtsplus 800.tsin one tsconfig), on an M4 Max, node 20.20.2, TypeScript 5.7.2,@typescript-eslint8.46.4:project warm parse x150project parse+typed x150projectService warm parse x150projectService parse+typed x150replaceExtensions over 400 .ts sourcesreplaceExtensions over 400 .ts sources importing .gtsRepeat runs put the parse rows in a 1.2x to 1.8x band depending on how quiet the machine is. The ordering is stable, but don't read the third digit.
The last row is a no-op case that lands inside the noise floor, and it has come out on both sides of parity: 1.02x faster here, 1.09x slower on the CI runner. Treat it as "no measurable difference" rather than as either a win or a regression. That is the expected shape, since all the pre-check adds to a file that does contain
.gtsis oneString.includesscan before the same parse as before.The last two rows are new scenarios in
tests/project.bench.mjs. Everything already there measures warm parses, which happen after the program is built, so none of it ever reached the patchedreadFileand that cost was invisible to the bench. The second of the two is the case the pre-check can't skip, included so the pair brackets the change instead of only measuring the half it was built to win.The gains land hardest in classic
projectmode, which is where the profile that started this pointed.projectServicekeeps source files in itsDocumentRegistryinstead of re-reading them, so it hits the patchedreadFilefar less often.On a real app
The synthetic project tops out well below the scale where this hurts, so I also measured a private Ember app: about 23,500 files in a single program, 3,700 of them
.gts, ontypescript@6.0.3,@typescript-eslint/parser@8.65.0andeslint@10.8.0. Both parser versions were resolved against that app's own dependency closure, so the only thing differing between the two runs ists-patch.js. Parse only, no rules running.mainAbout 13 ms per linted file, holding across two rounds with the run order reversed.
That lines up with the CPU profile that started this, which attributed 16.73 ms of self time per linted file to this parser in classic
projectmode, against a 57.9 ms per-file total for that mode: roughly 29% of type-aware lint time spent in the parser rather than in TypeScript. Recovering ~13 of those 16.73 ms is about what a parse-only measurement should show.The cold row is the other half. That profile was taken by differencing runs at n=200 and n=800 to cancel one-off project load, which is exactly what hides program construction:
replaceExtensionsruns there, once per file in the tsconfig rather than once per linted file, so it contributed ~0 to the differenced per-file figure while still costing ~2.8 s of every lint process. Worth knowing if you go looking for it in a profile and can't find it.Checks run
pnpm test,pnpm lint,pnpm --filter '*' test:check,pnpm --filter '*' test:fix(thegjs-typesproject lints under bothprojectandprojectService), andnode ./scripts/eslint-plugin-ember-test.mjs(6,327 tests).Two things beyond the suite, since "no behavior change" is most of the claim here:
The new
syncMtsGtsSourceFilestests were checked against a build with the memo guard deleted, and the pinning test fails there as it should. ThereplaceExtensionstests pass againstmain's implementation too, except the escaped-specifier ones, which is the divergence in §1.A differential harness parses a synthetic project of 25
.gtsplus 40.tsfiles three times over in classicprojectmode (passes after the first are what exercise the skip), dumpschecker.typeToString(getTypeAtLocation(...))for every ESTree node with a TS mapping, and appends every whole-program semantic diagnostic. 8,495 lines, byte-identical betweenmain'sts-patch.jsand this branch. The diagnostics include5097errors on rewritten.mtsspecifiers, so the rewrite path is genuinely covered rather than skipped.Investigated and written by Claude (Claude Code), at Peter Wagenet's request.
🤖 Generated with Claude Code