Skip to content

Use istr as keys in CIMultiDict#1097

Merged
asvetlov merged 29 commits intomasterfrom
istr-keys
Mar 26, 2025
Merged

Use istr as keys in CIMultiDict#1097
asvetlov merged 29 commits intomasterfrom
istr-keys

Conversation

@asvetlov
Copy link
Member

@asvetlov asvetlov commented Mar 22, 2025 •
edited
Loading

Currently, keys of CIMultiDict are stored as-is, e.g. CIMultiDict({'k': 'v'}) has str key.

But it is slightly incorrect, CIMultiDict should store istr keys.

The PR fixes it.

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 23, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #1097 will degrade performances by 54.53%

Comparing istr-keys (2e9d6e6) with master (10a97e1)

Summary

⚡ 26 improvements
❌ 1 (👁 1) regressions
✅ 115 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
⚡ test_cimultidict_extend_istr[pure-python-module] 150.4 ms 90.7 ms +65.81%
⚡ test_cimultidict_extend_istr_with_kwargs[pure-python-module] 199.9 ms 114.6 ms +74.47%
⚡ test_create_cimultidict_with_dict_istr[pure-python-module] 575.2 µs 318.2 µs +80.75%
⚡ test_create_cimultidict_with_items_istr[pure-python-module] 526 µs 269.7 µs +95.03%
⚡ test_create_cimultidict_with_items_istr_with_kwargs[pure-python-module] 1,044.8 µs 535 µs +95.29%
⚡ test_create_from_existing_cimultidict[pure-python-module] 63.5 µs 47.9 µs +32.46%
⚡ test_create_multidict_with_dict[case-insensitive-pure-python-module] 545.4 µs 302.8 µs +80.12%
⚡ test_create_multidict_with_dict[case-sensitive-pure-python-module] 481.5 µs 243.2 µs +97.97%
⚡ test_create_multidict_with_items[case-insensitive-pure-python-module] 499.2 µs 255.4 µs +95.48%
⚡ test_create_multidict_with_items[case-sensitive-pure-python-module] 433 µs 195.9 µs ×2.2
⚡ test_create_multidict_with_items_with_kwargs[case-insensitive-pure-python-module] 1,000.1 µs 511.3 µs +95.6%
⚡ test_create_multidict_with_items_with_kwargs[case-sensitive-pure-python-module] 868.9 µs 391 µs ×2.2
⚡ test_multidict_add_str[case-sensitive-pure-python-module] 24.2 ms 21.2 ms +14.26%
⚡ test_multidict_delitem_str[case-sensitive-pure-python-module] 1.5 ms 1.3 ms +14.16%
⚡ test_multidict_extend_str[case-insensitive-c-extension-module] 8.5 ms 7.7 ms +9.71%
⚡ test_multidict_extend_str[case-insensitive-pure-python-module] 143.5 ms 86.9 ms +65.04%
⚡ test_multidict_extend_str[case-sensitive-c-extension-module] 3.1 ms 2.2 ms +37.03%
⚡ test_multidict_extend_str[case-sensitive-pure-python-module] 124.4 ms 56.8 ms ×2.2
⚡ test_multidict_extend_str_with_kwargs[case-insensitive-c-extension-module] 13.6 ms 11.5 ms +18.35%
⚡ test_multidict_extend_str_with_kwargs[case-insensitive-pure-python-module] 191.2 ms 110.1 ms +73.69%
... ... ... ... ...

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

@codecov
Copy link

codecov bot commented Mar 23, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 59.22330% with 42 lines in your changes missing coverage. Please review.

Project coverage is 92.96%. Comparing base (10a97e1) to head (2e9d6e6).
Report is 73 commits behind head on master.

Files with missing lines Patch % Lines
multidict/_multidict_py.py 46.15% 0 Missing and 42 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1097      +/-   ##
==========================================
+ Coverage   92.86%   92.96%   +0.09%     
==========================================
  Files          44       44              
  Lines        5088     5188     +100     
  Branches      385      397      +12     
==========================================
+ Hits         4725     4823      +98     
  Misses         95       95              
- Partials      268      270       +2
Flag Coverage Δ
CI-GHA 92.96% <59.22%> (+0.09%) ⬆️
MyPy 86.75% <59.22%> (+0.17%) ⬆️
pytest 100.00% <ø> (ø)

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 23, 2025
View reviewed changes
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Mar 24, 2025
@asvetlov asvetlov marked this pull request as ready for review March 25, 2025 17:07
@asvetlov
Copy link
Member Author

asvetlov commented Mar 25, 2025

The regression is ok

@bdraco
Copy link
Member

bdraco commented Mar 25, 2025

The regression is ok

I think it’s fine as it’s a rare case.

will test on production later today

@bdraco
Copy link
Member

bdraco commented Mar 25, 2025 •
edited
Loading

Testing

  • aiohttp master
  • aiohttp 3.11
  • HA production 1
  • HA production 2

bdraco
bdraco reviewed Mar 25, 2025
View reviewed changes
@bdraco
Copy link
Member

bdraco commented Mar 25, 2025

Tested on production and as above. All seems to be working as expected.

Looked through the code changes and didn't see any issues stand out

@asvetlov @bdraco
Update CHANGES/1097.bugfix
2e9d6e6 
Co-authored-by: J. Nick Koston <nick@koston.org>
@asvetlov
Copy link
Member Author

asvetlov commented Mar 26, 2025

Thanks for the review, @bdraco

@asvetlov asvetlov enabled auto-merge (squash) March 26, 2025 08:26
@asvetlov asvetlov merged commit 389758e into master Mar 26, 2025
46 checks passed
@asvetlov asvetlov deleted the istr-keys branch March 26, 2025 08:35
@asvetlov asvetlov mentioned this pull request Mar 27, 2025
Merged
This was referenced Apr 8, 2025
Closed
Merged
bdraco added a commit that referenced this pull request Apr 9, 2025
@bdraco @asvetlov
Fix memory leak creating new istr objects (#1133)
78761cf 
Fixes leak 1 from
#1131 (comment)
(`IStr_New` path)

see
#1131 (comment)
#1131 (comment)
#1131 (comment)

This also seems to fix the perf regression in popitem from
#1097 (comment)
https://codspeed.io/aio-libs/multidict/branches/istr-keys

introduced #1097
fixes #1131

---------

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bdraco bdraco bdraco approved these changes

Assignees

No one assigned

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@asvetlov @bdraco