-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
check-manifest
became super slow
#141
Comments
I don't know what to say. All other changes in 0.46 were trivial cleanups/refactorings and the switch from Travis CI to GitHub Actions. I wonder if it would be useful to have check-manifest print the times it took to build the sdists in verbose mode, but of course that wouldn't help the comparisons. I suppose you could use strace with timings enabled... |
Thanks for your reply! I never used strace before, but this looks not so good :-/
Then I learned about the new option Then it looks like
Way better! You can reproduce this with https://github.com/jugmac00/flask-reuploaded Actually, I am happy again with |
Huh, it looks like you're stracing pre-commit rather than check-manifest. I was hoping to see the clone()/execve()/wait() from check-manifest invoking the actual sdist build thing (python setup.py sdist or the pep-517 thing I forget what it's called). If you pass Anyway, it looks like you've identified that the main slowdown is due to using isolated PEP-517 builds. And as for 0.45 -> 0.46 regression, is there any chance it can be explained by measurement errors (due to CPU frequency scaling or something)? |
You see pre-commit reading check-manifest's output from a pipe (fd 4). The timestamp indicates when the read() syscall started, you might want to also pass I cannot explain why the next read returns EIO rather than 0 (for EOF) or EAGAIN (in case pre-commit uses nonblocking I/O). Were there any syscalls between the two read() calls? Are you on Linux? |
Hi Marius, I repeated the strace with I lack both the knowledge and the time to get anything useful out of the output. As I found a working solution ( Just when I wanted to close the issue, I saw that someone else linked to this issue - see above my comment. Up to you what you do with this issue - if you want me to do perform any tests, just say so, but I am afraid I do not have the time to learn to interpret the output of strace currently. |
Heh, yes, strace files are something. I wrote strace-process-tree so I wouldn't have to interpret them myself. Now strace-process-tree doesn't like this format, but after some careful massaging with vim to have a pid on every line and avoid 'strace: Process NNN attached' messages breaking
As you can see, there are two That second of overhead is something that I would be interested in optimizing, if I had the time. The two |
Unfortunately, I don't have the time to look into this right now but wanted to say that I have also experienced this same issue. As you can see below I am getting similar results to @jugmac00 for version 0.47 $ time check-manifest --version
check-manifest version 0.47
check-manifest --version 0.17s user 0.06s system 98% cpu 0.235 total
$ time check-manifest
lists of files in version control and sdist match
check-manifest 8.31s user 2.35s system 92% cpu 11.560 total |
For e.g. https://github.com/jugmac00/flask-reuploaded (but also for my other projects), I notice an increased runtime for the checks.
The biggest jump was from version
0.40
to0.41
, but it looks like it is getting slower with each release...I do run
check-manifest
viapre-commit
, but I could also reproduce the problem with a standalone version ofcheck-manifest
.I only had a quick look and looks like the
pyproject.toml
support could have introduced the degraded performance. The runtime difference between0.45
and0.46
can be hardly explained by the changelog.tests without pre-commit
0.40
0.41
0.45
0.46
So, the changelog maybe is not really complete - as even without using
pre-commit
there is a performance degradation between 0.45 and 0.46 which only changelog entry sayspre-commit
now uses Python 3.The text was updated successfully, but these errors were encountered: