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

PERF: remove isdir() check in copy_file(), about 10% of the run time was checking if the file to copy is a directory. #4406

Closed
wants to merge 1 commit into from

Conversation

morotti
Copy link

@morotti morotti commented May 30, 2024

Hello,

I was doing a bit of profiling on my CI builds. The step to build package (python setup.py bdist_wheel) is taking more time than I would like.

Going in with a commercial profiler, I am finding that nearly 10% of the time is taken in isdir() calls, mostly in the copy_file() function.
Which is quite surprising IMO because the copy file function should not be given directories anyway :D

Can I offer to remove the isdir() calls to get a speedup?
I've found 2 calls that were passing directories instead of a filepath and corrected them.

By the way, the whole _copy_file_contents() could be replaced by shutil.copyfile() for another little boost. The code goes back to year 2000 or before, which predates optimized shutil functions.

Screenshot of the profiler run. The whole run is about 20+ seconds.
image

Regards.

…was checking if the file to copy is a directory.
@abravalheri
Copy link
Contributor

abravalheri commented May 30, 2024

Hi @morotti, thank you for having a look on this.
It is nice to have performance optimisations!

The only thing that I would say is that the development of the setuptools/_distutils folder is not done in this repository.
PRs that change those files first need to be merged in https://github.com/pypa/distutils and then we pull them in setuptools.

So I would recommend submitting the PR to https://github.com/pypa/distutils and discussing with the maintainers of that repo.

I enabled the CI on this PR so you can also use as an argument that "it does not break anything".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants