Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix 1.3 performance regression when locking with PyPI repository #7232

Merged

Conversation

radoering
Copy link
Member

I noticed a significant performance regression when locking the shootout example with a warm cache. The regression is caused by the removal of cachy (#5868) and has been fixed only for legacy repositories in #6932. I refactored _get_page a bit so that the same fix applies to PyPI repositories.

poetry time for poetry lock with warm cache
1.2.2 17 s
1.3.1 38 s
this PR 17 s

@dimbleby
Copy link
Contributor

Huh, I'd considered this but convinced myself that the cachecontrol cache was fast enough. Can't argue with facts though, nice win.

assert isinstance(json_page, SimpleJsonPage) alerts my smell-detector, there are a couple of curious things going on here

  • we only need specifically a SimpleJsonPage so that we can go pretty_name = json_page.content["name"]. However since the name that the JSON API returns is normalized this is pointless: we're not succeeding in extracting a pretty name anyway and might as well not bother
  • then it would be enough to assert json_page is not None: which raises the question of why the other _get_page methods return an optional thing whereas the pypi one returns a definite thing.

@radoering radoering force-pushed the fix-performance-get-page branch from e32e52f to 4222389 Compare January 1, 2023 10:05
@radoering radoering added impact/backport Requires backport to stable branch backport/1.3 impact/changelog Requires a changelog entry labels Jan 1, 2023
@neersighted neersighted added this to the 1.4 milestone Jan 1, 2023
…gacy repository to fix performance regression caused by replacing cachy
…ad of returning None; remove unsuccessful attempt to get pretty name via json-based simple API
@neersighted neersighted force-pushed the fix-performance-get-page branch from 4222389 to b26455f Compare January 1, 2023 22:29
@neersighted neersighted enabled auto-merge (rebase) January 1, 2023 22:29
@neersighted neersighted merged commit bfd490c into python-poetry:master Jan 1, 2023
@poetry-bot
Copy link

poetry-bot bot commented Jan 1, 2023

The backport to 1.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport-7232-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 bfd490c2ca3a8ff0d73b71ba355454df88868627
# Push it to GitHub
git push --set-upstream origin backport-7232-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3

Then, create a pull request where the base branch is 1.3 and the compare/head branch is backport-7232-to-1.3.

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
@radoering radoering deleted the fix-performance-get-page branch November 24, 2024 12:46
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
impact/backport Requires backport to stable branch impact/changelog Requires a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants