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

Improve performance of uv-python crate's manylinux submodule #11131

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cthoyt
Copy link
Contributor

@cthoyt cthoyt commented Jan 31, 2025

Summary

This PR makes a few performance improvements:

  1. Reduces the need to unpack and repack a _GLibCVersion tuple
  2. Reduces the doubled call to _is_compatible(arch, glibc_version)
  3. Moves the assignment of the tag variable directly into the yield, reducing memory allocation in case this is never used when _is_compatible(arch, glibc_version) is false
  4. Combines the check of the glibc_version being in _LEGACY_MANYLINUX_MAP and the assignment to the variable together. I'm not sure if this is actually better, but using the assignment expression reduces this from 4 lines to 2

Test Plan

I upstreamed these changes in pypa/packaging#869. Otherwise, I'm pretty confident this is a minor change that works the same

@charliermarsh
Copy link
Member

Thanks for this. I'm hesitant to make changes here since the code is vendored... Personally, I'd prefer to wait for this to merge upstream.

@cthoyt
Copy link
Contributor Author

cthoyt commented Jan 31, 2025

@charliermarsh totally agreed! I didn't realize it was vendored until after I had already made the PR... sorry for missing the README. I will watch out to see what the upstream group says, and ping you if we ever get some feedback!

Cheers

@charliermarsh
Copy link
Member

Thank you!

@charliermarsh charliermarsh added performance Potential performance improvement upstream An upstream dependency is involved labels Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Potential performance improvement upstream An upstream dependency is involved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants