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

Move lru cache from inside of _encode_host to outside #1348

Merged
merged 32 commits into from
Oct 21, 2024

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Oct 21, 2024

Everything ended up being cached on the inside of encode_host so it makes more sense to have a single cache for encoding hosts.

The ip_address, and host_validate caches are now deprecated in favor of a single encode_host cache.

For backwards compatibility cache_configure still accepts the old parameters but will use them to adjust the encode_host cache size instead. Additionally cache_info still returns the keys for the old options but will use the encode_host values.

Everything ended up being cached on the inside of the function so
it makes more sense to have a single cache for hosts
@bdraco
Copy link
Member Author

bdraco commented Oct 21, 2024

Will need some docs changes as well to deprecate the old cache keys

@bdraco
Copy link
Member Author

bdraco commented Oct 21, 2024

Will need to adjust coveragerc for the less lines

Copy link

codspeed-hq bot commented Oct 21, 2024

CodSpeed Performance Report

Merging #1348 will improve performances by 40.24%

Comparing move_cache_encode_host (aa9afb4) with master (04f382f)

Summary

⚡ 7 improvements
✅ 76 untouched benchmarks

Benchmarks breakdown

Benchmark master move_cache_encode_host Change
test_update_query_sequence_mapping 3.8 ms 3.5 ms +7.61%
test_url_build_with_different_hosts 1.8 ms 1.3 ms +40.24%
test_url_make_with_ipv6_address_and_path 768.4 µs 707.9 µs +8.55%
test_url_make_with_ipv6_address_path_and_port 847.8 µs 797.1 µs +6.35%
test_url_make_with_many_hosts 3.9 ms 3.6 ms +8.27%
test_url_make_with_many_ipv4_hosts 4.1 ms 3.6 ms +13.67%
test_url_make_with_many_ipv6_hosts 5.2 ms 4.7 ms +10.3%

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.14%. Comparing base (04f382f) to head (aa9afb4).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1348      +/-   ##
==========================================
+ Coverage   96.13%   96.14%   +0.01%     
==========================================
  Files          27       27              
  Lines        5539     5557      +18     
  Branches      357      358       +1     
==========================================
+ Hits         5325     5343      +18     
  Misses        188      188              
  Partials       26       26              
Flag Coverage Δ
CI-GHA 96.14% <100.00%> (+0.01%) ⬆️
MyPy 45.43% <97.53%> (+0.21%) ⬆️
OS-Linux 99.23% <100.00%> (+<0.01%) ⬆️
OS-Windows 99.27% <100.00%> (+<0.01%) ⬆️
OS-macOS 98.96% <100.00%> (+<0.01%) ⬆️
Py-3.10.11 98.94% <100.00%> (+<0.01%) ⬆️
Py-3.10.15 99.18% <100.00%> (+<0.01%) ⬆️
Py-3.11.10 99.18% <100.00%> (+<0.01%) ⬆️
Py-3.11.9 98.94% <100.00%> (+<0.01%) ⬆️
Py-3.12.7 99.18% <100.00%> (+<0.01%) ⬆️
Py-3.13.0 99.18% <100.00%> (+<0.01%) ⬆️
Py-3.9.13 98.90% <100.00%> (+<0.01%) ⬆️
Py-3.9.20 99.14% <100.00%> (+<0.01%) ⬆️
Py-pypy7.3.16 99.18% <100.00%> (+<0.01%) ⬆️
Py-pypy7.3.17 99.20% <100.00%> (+<0.01%) ⬆️
VM-macos-latest 98.96% <100.00%> (+<0.01%) ⬆️
VM-ubuntu-latest 99.23% <100.00%> (+<0.01%) ⬆️
VM-windows-latest 99.27% <100.00%> (+<0.01%) ⬆️
pytest 99.23% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bdraco bdraco changed the title Move lru cache from inside of _encode_host to outside Move lru cache from inside of _encode_host to outside Oct 21, 2024
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 21, 2024
CHANGES/1348.breaking.rst Outdated Show resolved Hide resolved
yarl/_url.py Outdated Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Oct 21, 2024

Maybe safer to keep the idna encode cache as is in case the user has a lot of idna hosts

#476

docs/api.rst Outdated Show resolved Hide resolved
docs/api.rst Outdated Show resolved Hide resolved
docs/api.rst Outdated Show resolved Hide resolved
docs/api.rst Show resolved Hide resolved
CHANGES/1348.breaking.rst Outdated Show resolved Hide resolved
docs/api.rst Outdated Show resolved Hide resolved
docs/api.rst Outdated Show resolved Hide resolved
docs/api.rst Outdated Show resolved Hide resolved
yarl/_url.py Outdated Show resolved Hide resolved
yarl/_url.py Show resolved Hide resolved
CHANGES/1348.breaking.rst Outdated Show resolved Hide resolved
CHANGES/1348.breaking.rst Outdated Show resolved Hide resolved
CHANGES/1348.breaking.rst Outdated Show resolved Hide resolved
CHANGES/1348.breaking.rst Outdated Show resolved Hide resolved
CHANGES/1348.breaking.rst Outdated Show resolved Hide resolved
@bdraco bdraco marked this pull request as ready for review October 21, 2024 04:03
@bdraco bdraco merged commit db92ab5 into master Oct 21, 2024
43 of 46 checks passed
@bdraco bdraco deleted the move_cache_encode_host branch October 21, 2024 04:03
bdraco added a commit that referenced this pull request Oct 21, 2024
There was a refactoring error in #1348 that set this to the decode size

Fix the bug and add coverage to ensure it does not break again.

This change never made it to a release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant