Uh oh!
There was an error while loading. Please reload this page.
perf(coverage): normalize a path with one fork instead of four - #1105
Merged
Conversation
normalize_path is what the DEBUG trap calls on every cache miss -- 494 times in a single run of one test file, because the caches live in shell variables and each test body is a subshell. Its body was echo "$(cd "$(dirname "$file")" && pwd)/$(basename "$file")" which is four command substitutions. Doing the dirname and basename with parameter expansion leaves one: 2081ms to 424ms per 500 calls, and a --coverage run of tests/unit/assert/basic_test.sh from 6.4s to 4.7s, with the same hit count. The cd and pwd stay. They resolve symlinks, so /tmp/x normalizes to /private/tmp/x on macOS the way the tracked roots expect; pure string manipulation would be faster still and would quietly stop matching them. builtin printf rather than printf, so a test spying printf cannot shadow a path the engine depends on -- the #724 test caught exactly that. Closes#1102
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 freeto 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.
🤔 Background
Related #1102
normalize_pathis what the DEBUG trap calls on every cache miss — 494 timesin a single run of one test file, because the caches live in shell variables
and each test body is a subshell. Its body was
four command substitutions per call.
💡 Changes
dirname/basenamebecome parameter expansions, leaving one fork: 2081 ms → 424 ms per 500 calls, and a--coveragerun ofbasic_test.sh6.4 s → 4.7 s with the same hit countcd+pwdstays deliberately: it resolves symlinks, so/tmp/xnormalizes to/private/tmp/xon macOS the way the tracked roots expect. Pure string manipulation is faster still and would quietly stop matching thembuiltin printf, notprintf, so a test spyingprintfcannot shadow a path the engine depends on — the Spying on printf breaks coverage collection #724 test caught exactly that during developmentNote the issue's original premise (path mangling dominates) was wrong and is
corrected in the thread: the memo it proposed bought 4%, this buys 27%.