Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Walk the path backwards when creating new directories #1057

Merged
merged 3 commits into from
Jul 14, 2024

Conversation

rydrman
Copy link
Collaborator

@rydrman rydrman commented Jun 21, 2024

Our original algorithm walked down from the root, creating directories as needed. This creates an operation of O(n) under all scenarios, where n is the number of path components. However, we can generally assume that these paths are going to be created once and then already exist from that point forward. This mean that for a vast majority of the operations, we are over-checking. This new system walks up from the full path, checking each entry and tracking which do not exist. Once it finds an existing parent, it walks back down creating each item. In a worst case scenario, this becomes an O(2n) operation but changes our best-case to an O(1). Eg if the path already exists, a single lstat call confirms this and nothing else needs to be done. This feels like a desirable trade-off given that most operations are likely to fall into the "already exists" bucket.

@rydrman rydrman added the enhancement New feature or request label Jun 21, 2024
@rydrman rydrman requested a review from jrray June 21, 2024 20:49
@rydrman rydrman self-assigned this Jun 21, 2024
@rydrman rydrman force-pushed the reduced-steps-makedir-with-perms branch 2 times, most recently from b1ed27b to 1ec44c3 Compare June 21, 2024 22:02
@rydrman
Copy link
Collaborator Author

rydrman commented Jun 21, 2024

Not sure exactly what changed with the integration tests here but I will chase it

@rydrman rydrman force-pushed the reduced-steps-makedir-with-perms branch 2 times, most recently from 85e8fea to 1b88abe Compare June 21, 2024 23:13
@rydrman rydrman changed the base branch from main to eaccess-on-fedora June 21, 2024 23:13
@rydrman rydrman requested a review from jrray June 22, 2024 15:12
@rydrman rydrman force-pushed the eaccess-on-fedora branch 2 times, most recently from 961ce75 to 0a6f428 Compare June 22, 2024 15:17
Our original algorithm walked down from the root, creating directories as
needed. This creates an operation of O(n) under all scenarios, where n
is the number of path components. However, we can generally assume
that these paths are going to be created once and then already exist
from that point forward. This mean that for a vast majority of the
operations, we are over-checking. This new system walks up from the full
path, checking each entry and tracking which do not exist. Once it
finds an existing parent, it walks back down creating each item. In a
worst case scenario, this becomes an O(2n) operation but changes our
best-case to an O(1). Eg if the path already exists, a single lstat call
confirms this and nothing else needs to be done. This feels like a
desirable trade-off given that most operations are likely to fall into
the "already exists" bucket.

Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>
Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>
Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>
@rydrman rydrman force-pushed the reduced-steps-makedir-with-perms branch from c1ba84e to fcd7e60 Compare June 22, 2024 15:17
@rydrman rydrman changed the base branch from eaccess-on-fedora to main June 22, 2024 15:17
@rydrman
Copy link
Collaborator Author

rydrman commented Jul 14, 2024

@jrray FYI I'm not sure if you are planning to review this again since we talked about it...

Copy link
Collaborator

@jrray jrray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I never approve it? Sorry about that.

@rydrman rydrman merged commit 2ffe92e into main Jul 14, 2024
7 checks passed
@rydrman rydrman deleted the reduced-steps-makedir-with-perms branch July 14, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants