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

Replace self._val with separate properties #1397

Merged
merged 30 commits into from
Oct 30, 2024
Merged

Replace self._val with separate properties #1397

merged 30 commits into from
Oct 30, 2024

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Oct 30, 2024

self._val has been eliminated and replaced with self._scheme, self._netloc, self._path, self._query, self._fragment

Second part of #1396

Closes #172

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

codspeed-hq bot commented Oct 30, 2024

CodSpeed Performance Report

Merging #1397 will improve performances by 57.36%

Comparing elim_val (0ff04a4) with master (41af4a3)

Summary

⚡ 19 improvements
✅ 63 untouched benchmarks

Benchmarks breakdown

Benchmark master elim_val Change
test_empty_query 50 µs 46.7 µs +7.07%
test_empty_query_string 50.8 µs 46.4 µs +9.53%
test_url_equality 358.1 µs 310.7 µs +15.23%
test_url_join 320.5 µs 261.4 µs +22.58%
test_url_make_access_fragment 247.3 µs 167.9 µs +47.35%
test_url_make_access_raw_host 247.8 µs 168.2 µs +47.3%
test_url_make_access_raw_path 246.8 µs 167.3 µs +47.49%
test_url_make_access_username_password 272.5 µs 193.5 µs +40.82%
test_url_make_empty_password 218.1 µs 139.1 µs +56.81%
test_url_make_empty_username 219.4 µs 140.4 µs +56.3%
test_url_make_encoded_with_host_path_and_port 269.9 µs 188.4 µs +43.3%
test_url_make_no_netloc 218.1 µs 138.9 µs +57%
test_url_make_no_netloc_relative 218.6 µs 139.1 µs +57.21%
test_url_make_with_host_and_path 219 µs 139.5 µs +57%
test_url_make_with_host_path_and_port 218.6 µs 139 µs +57.33%
test_url_make_with_ipv4_address_and_path 218.1 µs 139.7 µs +56.07%
test_url_make_with_ipv4_address_path_and_port 218.7 µs 140.1 µs +56.11%
test_url_make_with_ipv6_address_and_path 218.1 µs 139.2 µs +56.75%
test_url_make_with_ipv6_address_path_and_port 218.7 µs 139 µs +57.36%

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.05%. Comparing base (41af4a3) to head (0ff04a4).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1397      +/-   ##
==========================================
+ Coverage   96.01%   96.05%   +0.03%     
==========================================
  Files          31       31              
  Lines        5692     5748      +56     
  Branches      353      342      -11     
==========================================
+ Hits         5465     5521      +56     
  Misses        201      201              
  Partials       26       26              
Flag Coverage Δ
CI-GHA 96.05% <100.00%> (+0.03%) ⬆️
MyPy 48.83% <78.88%> (+<0.01%) ⬆️
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.51% <100.00%> (+<0.01%) ⬆️
Py-3.11.10 99.51% <100.00%> (+<0.01%) ⬆️
Py-3.11.9 99.27% <100.00%> (+<0.01%) ⬆️
Py-3.12.7 99.51% <100.00%> (+<0.01%) ⬆️
Py-3.13.0 99.51% <100.00%> (+<0.01%) ⬆️
Py-3.9.13 99.23% <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.55% <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.

@bdraco
Copy link
Member Author

bdraco commented Oct 30, 2024

This seems slower and less readable

@bdraco
Copy link
Member Author

bdraco commented Oct 30, 2024

Going to try one more thing before I throw in the towel on this one

@bdraco
Copy link
Member Author

bdraco commented Oct 30, 2024

Thats a lot more readable.... lets see if its +/- on performance though

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

bdraco commented Oct 30, 2024

still 8% slower making the URL
https://codspeed.io/aio-libs/yarl/branches/elim_val

If we move the URL inside the pre_encoded_url and encode_url we can eliminate all the setattrs

@bdraco
Copy link
Member Author

bdraco commented Oct 30, 2024

that works but need to make sure we do not cache on default value since unpickling mutates the object and everything else expected URL is immutable.

@bdraco bdraco changed the title Dnm: Fully eliminate self._val Replace self._val with separate properties Oct 30, 2024
@bdraco bdraco marked this pull request as ready for review October 30, 2024 16:09
@bdraco bdraco merged commit 3f2c26f into master Oct 30, 2024
44 of 46 checks passed
@bdraco bdraco deleted the elim_val branch October 30, 2024 16:09
bdraco added a commit that referenced this pull request Oct 30, 2024
yarl/_url.py Show resolved Hide resolved
yarl/_url.py Show resolved Hide resolved
bdraco added a commit that referenced this pull request Nov 17, 2024
#1397 made this a less concise, clean it up a bit
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.

Replace _val with separate props for url parts
2 participants