Repository navigation
perf: stop re-resolving every prefix of a missing path - #175
Open
ferdingler wants to merge 1 commit into
Open
ferdingler wants to merge 1 commit into
ferdingler wants to merge 1 commit into
Conversation
Finding the bind above a path walked up one component at a time and resolved the whole prefix at each step, so a long path of missing components cost time quadratic in its length while holding the VFS lock. Nothing resolves past a bind and a shorter prefix resolves whenever a longer one does, so a binary search for the longest resolving prefix finds the same ancestor, and the missing tail is appended once. Adds unit tests that pin the existing answers (missing tails under a bind, '..' across a missing component, symlinks before a missing tail, refused host symlinks, no bind ancestor) and a test that compares the cost of 2,000 and 16,000 missing components.
This branch has not been deployed
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
1 participant
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.
Description
A long path made of components that don't exist was really slow to fail. About 32 KB of
a/a/a/...took around 85 seconds in a debug build, well under the default input limit. Looking up which bind a path sits under re-resolved the whole path once per missing component, so the cost grew with the square of the length, and the VFS lock was held the whole time.Now the lookup finds the deepest part of the path that exists in a handful of steps and appends the missing rest once. The answers don't change:
./..handling, symlinks, binds, and the paths that get refused all behave as before. The only code change is in that one lookup; the rest is tests. We already fixed this in Box's copy of Shell (here), and this brings the fix upstream.I added tests that pin the current answers for missing paths under a bind,
..across a missing component, symlinks in front of a missing tail, host symlinks that dangle or leave the bind, and paths with no bind. They pass on main too, and I broke the new search a couple of ways on purpose to check they catch it. I also added a timing test that compares 2,000 against 16,000 missing components in the same run, so it doesn't depend on how fast the runner is. On main the long case took about 85 seconds against 1.3 for the short one and the test failed. With the change the whole test takes about a tenth of a second. It only covers this path shape, though. It doesn't show the cost is linear for every kind of path, or for deep trees full of symlinks.This touches the same file as #170, but a different function, so they shouldn't conflict.
Related Issues
None.
Documentation PR
None.
Type of Change
Other (please describe): Performance fix
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.
cargo test --workspace --all-targets,pytest tests/python,npm test)cargo fmtandcargo clippyChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.