GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-119169: Skip reversing sibling directories in `os.[f]walk(topdown=False)` by barneygale · Pull Request #119473 · python/cpython

Conversation

@barneygale

@barneygale barneygale commented May 23, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

In os.walk(topdown=False), don't bother reversing walk_dirs. This means that sibling directories are visited in a different order, but 1) that order is arbitrary and comes from os.scandir(), and 2) unlike in top-down mode, users can't influence which directories are visited or in what order.

This change caused test_walk_bottom_up to fail. I think this test made assertions that were too specific and relied on os.scandir() returning things in a specific order. The test code is pretty hard to understand once you get into the details. I've replaced it with a version of the same test from test_pathlib_abc.py. The new test checks that every directory is yielded before all its ancestors, and after all its descendants, but does not check the order of its yielded siblings.

  • Issue: Improve performance of os.walk() #119169

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@barneygale
pythonGH-119169: Skip reversing sibling directories in `os.walk(topdo…
469a9cf
…wn=False)`

In `os.walk(topdown=False)`, don't bother reversing `walk_dirs`. This means
that sibling directories are visited in a different order, but 1) that
order is arbitrary and comes from `os.scandir()`, and 2) unlike in top-down
mode, users can't influence which directories are visited or in what order.

This change caused `test_walk_bottom_up` to fail. I think this test made
assertions that were too specific and relied on `os.scandir()` returning
things in a specific order, and the test code is pretty hard to understand
once you get into the details. I've replaced it with a version of the same
test from `test_pathlib_abc.py`.
@barneygale barneygale added performance Performance or resource usage skip news labels May 23, 2024
@bedevere-app bedevere-app Bot added the awaiting core review label May 23, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 23, 2024
Improve performance of os.walk() #119169
Open
This was referenced May 23, 2024
GH-119169: Speed up os.walk(topdown=False) #119186
Closed
GH-89727: Fix os.fwalk() recursion error on deep trees #119638
Merged
barneygale added 2 commits May 30, 2024 04:38
@barneygale
Merge branch 'main' into os-walk-bottom-reversal
f81864c
@barneygale
Also undo reversals in os.fwalk()
cfc60ce
@barneygale barneygale changed the title GH-119169: Skip reversing sibling directories in os.walk(topdown=False) GH-119169: Skip reversing sibling directories in os.[f]walk(topdown=False) May 30, 2024
@barneygale
Merge branch 'main' into os-walk-bottom-reversal
ca9c16a
@barneygale
barneygale requested a review from serhiy-storchaka July 6, 2024 13:15
@barneygale

barneygale commented Jul 6, 2024

Copy link
Copy Markdown
Contributor Author

Hey @serhiy-storchaka, just for context: this change doesn't provide much of a speedup in itself, but it enables more significant speedups seen in GH-119186. I've separated it out here because it's an observable change that breaks a test. But I suspect that doesn't matter as the scandir() result order is arbitrary and the order of sibling visits can't be influenced. What do you think? Thank you!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@barneygale barneygale mentioned this pull request Jul 6, 2024
GH-119169: Speed up os.fwalk(topdown=False) #121433
Closed
@serhiy-storchaka

serhiy-storchaka commented Jul 8, 2024

Copy link
Copy Markdown
Member

If os.walk(topdown=False) reverses the order of visiting directories, this should be fixed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@barneygale

barneygale commented Jul 8, 2024

Copy link
Copy Markdown
Contributor Author

It presently visits sibling directories in the same order they appear in dirnames.

What I'm proposing here is that we don't need to visit sibling directories in the same order they appear in dirnames in bottom-up mode, because:

  1. The order is arbitrary (it comes from os.scandir())
  2. Users aren't able to influence the order mid-walk by re-ordering dirnames (this is only possible in top-down mode)

Perhaps a worked example would be helpful? I can put one together.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@barneygale

barneygale commented Jul 8, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Here's an example, first with the current main:

>>> os.mkdir('mydir')
>>> os.mkdir('mydir/subdir1')
>>> os.mkdir('mydir/subdir2')
>>> for entry in os.walk('mydir', topdown=False):
...     print(entry)
...     
('mydir/subdir2', [], [])
('mydir/subdir1', [], [])
('mydir', ['subdir2', 'subdir1'], [])

And with this PR:

>>> for entry in os.walk('mydir', topdown=False):
...     print(entry)
...     
('mydir/subdir1', [], [])
('mydir/subdir2', [], [])
('mydir', ['subdir2', 'subdir1'], [])

Note how the entry for subdir1 is now yielded before subdir2, and how this no longer corresponds to the order in dirnames for mydir.

If we think the new behaviour is acceptable (e.g. because we never guaranteed the order of sibling visits) then it unlocks an optimization: we can add paths to the stack while scanning directories rather than in a separate loop afterwards.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Jul 8, 2024

Copy link
Copy Markdown
Member

I think that it would be confusing if os.walk(), grep.grep(), or manual recursive use of os.listdir() and os.scandir() produce different results. The order depends on the file system, but at least it is consistent.

The fact that the current code needs to reverse the lists of directories is just an implementation detail. It is not exposed to the user. If the code used deque instead of a list, it would not need to reverse them.

BTW, how much it makes difference? What it in comparison of using deque?

barneygale reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@barneygale

barneygale commented Jul 8, 2024

Copy link
Copy Markdown
Contributor Author

That makes sense, thanks.

BTW, how much it makes difference?

Taking into account the speedups in GH-119186 and GH-121433, it's about 1% for os.walk() and 6% for os.fwalk(). Nothing special.

What it in comparison of using deque?

I'll get back to you.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@barneygale

barneygale commented Jul 8, 2024

Copy link
Copy Markdown
Contributor Author

Hm, the deque would help with a breadth-first implementation, but I don't think it helps here without a lot of code changes. I'll close this PR and its two dependents.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@barneygale barneygale closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review performance Performance or resource usage skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@barneygale @serhiy-storchaka
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.