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

Complete removal of SplitResult as an internal #1396

Merged
merged 10 commits into from
Oct 30, 2024
Merged

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Oct 30, 2024

partially implements #172

It was suggested in #172 to only upgrade to SplitResult when needed. This will give us more flexibility to change how the internal structure works in the future and we won't be forever bound to using SplitResult.

The only place we can't fully get rid of it is for pickle without a breaking change. #172 suggested leaving it for compat, so thats what I did.

In the future we may want to change the split functions to unpack the values into named variables in future. ie self._scheme, self._netloc, self._path, self._query, self._fragment = val or store everything in self._cache as the single source of truth; However that was too large of a redesign for this PR since it would also require changing all the tuple unpacking and pre cache in __new__.

One consideration about getting rid of the _val tuple is that SplitResult is exposed as an allowed incoming value in new we would have to have code to convert it. With _val being a normal tuple, SplitResult can be passed in and its still compatible.

It would be a lot nicer if we stored everything in self._cache though since it means the encode_url and pre_encoded_url calls would return a simple dict that was pre-populated, and it also means propcache would have more keys pre-populated.

@bdraco bdraco changed the title Complete removal of SplitResult as an internal Complete removal of SplitResult as an internal Oct 30, 2024
yarl/_url.py Show resolved Hide resolved
yarl/_url.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.01%. Comparing base (24f814d) to head (facbe5b).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1396   +/-   ##
=======================================
  Coverage   96.00%   96.01%           
=======================================
  Files          31       31           
  Lines        5683     5692    +9     
  Branches      353      353           
=======================================
+ Hits         5456     5465    +9     
  Misses        201      201           
  Partials       26       26           
Flag Coverage Δ
CI-GHA 96.01% <100.00%> (+<0.01%) ⬆️
MyPy 48.83% <63.75%> (+0.06%) ⬆️
OS-Linux 99.55% <100.00%> (+<0.01%) ⬆️
OS-Windows 99.61% <100.00%> (+<0.01%) ⬆️
OS-macOS 99.29% <100.00%> (+<0.01%) ⬆️
Py-3.10.11 99.27% <100.00%> (+<0.01%) ⬆️
Py-3.10.15 99.50% <100.00%> (+<0.01%) ⬆️
Py-3.11.10 99.50% <100.00%> (+<0.01%) ⬆️
Py-3.11.9 99.27% <100.00%> (+<0.01%) ⬆️
Py-3.12.7 99.50% <100.00%> (+<0.01%) ⬆️
Py-3.13.0 99.50% <100.00%> (+<0.01%) ⬆️
Py-3.9.13 99.22% <100.00%> (+<0.01%) ⬆️
Py-3.9.20 99.46% <100.00%> (+<0.01%) ⬆️
Py-pypy7.3.16 99.52% <100.00%> (+<0.01%) ⬆️
Py-pypy7.3.17 99.54% <100.00%> (+<0.01%) ⬆️
VM-macos-latest 99.29% <100.00%> (+<0.01%) ⬆️
VM-ubuntu-latest 99.55% <100.00%> (+<0.01%) ⬆️
VM-windows-latest 99.61% <100.00%> (+<0.01%) ⬆️
pytest 99.55% <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.

Copy link

codspeed-hq bot commented Oct 30, 2024

CodSpeed Performance Report

Merging #1396 will improve performances by ×2.2

Comparing get_rid_splitresult (facbe5b) with master (24f814d)

Summary

⚡ 34 improvements
✅ 48 untouched benchmarks

Benchmarks breakdown

Benchmark master get_rid_splitresult Change
test_update_query_empty 125.9 µs 94.3 µs +33.57%
test_update_query_none 122.7 µs 90.9 µs +35.03%
test_url_build_access_fragment 607.8 µs 507.1 µs +19.86%
test_url_build_access_raw_host 717.6 µs 616.6 µs +16.37%
test_url_build_access_raw_path 666.5 µs 564.4 µs +18.1%
test_url_build_access_username_password 799.8 µs 697.8 µs +14.61%
test_url_build_encoded_with_host_and_port 500.6 µs 405.3 µs +23.52%
test_url_build_no_netloc 448.4 µs 356.9 µs +25.63%
test_url_build_no_netloc_relative 447.3 µs 354.3 µs +26.27%
test_url_build_with_different_hosts 1.3 ms 1 ms +22.87%
test_url_build_with_host 504.1 µs 410 µs +22.96%
test_url_build_with_host_and_port 717.7 µs 617.6 µs +16.21%
test_url_build_with_host_path_and_port 636.6 µs 537.9 µs +18.35%
test_url_equality 780.1 µs 358 µs ×2.2
test_url_extend_query_existing_query_string 224.3 µs 194 µs +15.65%
test_url_hash 323.3 µs 195 µs +65.77%
test_url_join 469.2 µs 320.4 µs +46.43%
test_url_joinpath_encoded 871.8 µs 751.4 µs +16.01%
test_url_joinpath_with_truediv 1,012.9 µs 883.1 µs +14.7%
test_url_to_string 308 µs 239.8 µs +28.45%
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

tests/test_pickle.py Outdated Show resolved Hide resolved
yarl/_url.py Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Oct 30, 2024

For the future we could store everything in self._cache as the single source of truth.

That would get rid of the tuple.. but thats a major change, and since SplitResult is
exposed as an allowed incoming value in new we would have to have code to convert it

It would be a lot nicer though since it means the encode_url and unencoded_url calls
would return a simple dict that was pre-populated, and it also means propcache
would have more keys pre-populated

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 30, 2024
@bdraco
Copy link
Member Author

bdraco commented Oct 30, 2024

#1397 Aims to fully eliminate Val and only construct it lazily

@bdraco bdraco marked this pull request as ready for review October 30, 2024 13:04
@bdraco bdraco merged commit 41af4a3 into master Oct 30, 2024
52 of 58 checks passed
@bdraco bdraco deleted the get_rid_splitresult branch October 30, 2024 13:05
bdraco added a commit that referenced this pull request Oct 30, 2024
bdraco added a commit that referenced this pull request Oct 30, 2024
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