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

use scandir() in find_commands.py to avoid stat() calls #13070

Closed
wants to merge 3 commits into from

Conversation

dholth
Copy link
Contributor

@dholth dholth commented Sep 7, 2023

Description

This function was especially slow on WSL. In addition to the previous fix, usescandir() which should grab the is_file attribute without a separate syscall in most cases.

Fixes #13067

See also #13071 which is backported to the 23.7.x branch.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@dholth dholth requested a review from a team as a code owner September 7, 2023 14:02
@dholth dholth self-assigned this Sep 7, 2023
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Sep 7, 2023
@jezdez jezdez changed the base branch from main to 23.7.x September 7, 2023 14:29
@jezdez jezdez changed the base branch from 23.7.x to main September 7, 2023 14:29
jezdez
jezdez previously requested changes Sep 7, 2023
Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

Since this is a regression in 23.7.x this should be targetting the 23.7.x branch and not main

Otherwise this seems okay

if m and isfile(join(dir_path, fn)):
for entry in os.scandir(dir_path):
m = pat.match(entry.name)
if m and entry.is_file():
Copy link
Member

Choose a reason for hiding this comment

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

Should this follow symlinks (the default)? Otherwise:

Suggested change
if m and entry.is_file():
if m and entry.is_file(follow_symlinks=False):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

os.path.isfile() follows symlinks. IMO you would expect to be able to symlink a conda-* command into PATH.

Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

@dholth dholth changed the base branch from main to 23.7.x September 7, 2023 14:41
@dholth dholth dismissed jezdez’s stale review September 7, 2023 14:42

Matches previous behavior.

@dholth dholth changed the base branch from 23.7.x to main September 7, 2023 14:42
@dholth
Copy link
Contributor Author

dholth commented Sep 7, 2023

@jezdez what if we target main and cherry-pick the fix into the release branch?

@kenodegard
Copy link
Contributor

kenodegard commented Sep 7, 2023

what if we target main and cherry-pick the fix into the release branch?

we can, I think it makes more sense (git graph wise) to target the release branch and then merge this back into main

@jezdez
Copy link
Member

jezdez commented Sep 7, 2023

We usually do forward ports not backports for release regressions, that's why we have the release branches. Since we can't push to branches directly, cherry-picking doesn't much make it a shortcut. You'd have to open another PR to apply the cherry-picked commit to the release branch. In the forward port scenario, we can do multiple regression fixes in one PR to merge back into main, saving a little time.

@dholth
Copy link
Contributor Author

dholth commented Sep 11, 2023

FYI the main branch has a pre-existing community contributed fix for this regression. #13035

@kenodegard
Copy link
Contributor

superseded by 23.7.4 and #13094

@kenodegard kenodegard closed this Sep 12, 2023
@kenodegard kenodegard deleted the find-commands-scandir branch September 26, 2023 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Conda 23.7.3 start-up time extremely slow compared to 23.5.2
4 participants