Add fast paths to CombinePaths and defer path canonicalization in checkSourceFilesBelongToPath - #63896
Draft
no-yan wants to merge 1 commit into
Draft
Add fast paths to CombinePaths and defer path canonicalization in checkSourceFilesBelongToPath#63896no-yan wants to merge 1 commit into
no-yan wants to merge 1 commit into
Conversation
…ckSourceFilesBelongToPath CombinePaths returned via a string builder even when the last argument is already slash-rooted (the common case for ContainsPath and module resolution callers); return it directly instead. Drive-letter and URL-rooted last arguments still take the general case, which is kept byte-for-byte unchanged as combinePathsSlow and doubles as the differential oracle for the exhaustive test. checkSourceFilesBelongToPath computed the canonical absolute path for every file even though it is only used in the error branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
no-yan please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Author
|
@microsoft-github-policy-service agree |
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
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.
Part of a series reducing allocation churn on hot paths (see #63895 for
motivation and series-wide measurements).
This PR
CombinePathsbuilt its result through astrings.Buildereven whenthe last argument is already an absolute path — the common case for
ContainsPathand module resolution callers, where the trailing pathsupersedes everything before it. Return it directly instead. Only
slash-rooted paths (without backslashes) take the fast path;
drive-letter and URL-rooted arguments still go through the general
case, which is kept byte-for-byte unchanged as
combinePathsSlowanddoubles as the differential oracle for the test.
checkSourceFilesBelongToPathcomputed the canonical absolute path forevery source file even though it is only used when emitting the error
diagnostic; compute it in the error branch instead.
Results
Two cases covering the fast paths are added to the existing
BenchmarkCombinePaths(benchstat, 8 alternated runs):An isolated microbenchmark of the miss path showed ≈+1 ns per call;
at ~125k relative-path calls per full vscode/src check that is ~0.1 ms
total. End-to-end,
CombinePathsallocations drop from 185k–207k to125k per full check.
Verification
TestCombinePathsMatchesSlowPathexhaustively comparesCombinePathsagainst
combinePathsSlow(the unmodified pre-fast-pathimplementation) over ~5,800 combinations of representative path
pieces, including backslashes, UNC, drive letters, URLs, and empties.
🤖 Generated with Claude Code