Skip to content

Fix memory leak creating new istr objects#1133

Merged
bdraco merged 7 commits intomasterfrom
leak_ci_arg_to_key
Apr 9, 2025
Merged

Fix memory leak creating new istr objects#1133
bdraco merged 7 commits intomasterfrom
leak_ci_arg_to_key

Conversation

@bdraco
Copy link
Member

@bdraco bdraco commented Apr 9, 2025

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

@bdraco bdraco changed the title Fix leak creating new istr objects Fix memory leak creating new istr objects Apr 9, 2025
@codecov
Copy link

codecov bot commented Apr 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.68%. Comparing base (9850454) to head (90b2507).
Report is 91 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1133   +/-   ##
=======================================
  Coverage   98.68%   98.68%           
=======================================
  Files          27       27           
  Lines        3432     3432           
  Branches      528      528           
=======================================
  Hits         3387     3387           
  Misses         17       17           
  Partials       28       28           
Flag Coverage Δ
CI-GHA 98.68% <ø> (ø)
MyPy 83.70% <ø> (ø)
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.

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 9, 2025

CodSpeed Performance Report

Merging #1133 will improve performances by 42.32%

Comparing leak_ci_arg_to_key (90b2507) with master (9850454)

Summary

⚡ 1 improvements
✅ 243 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
test_multidict_popitem_str[case-insensitive-c-extension-module] 160.4 µs 112.7 µs +42.32%

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Apr 9, 2025
@bdraco bdraco marked this pull request as ready for review April 9, 2025 04:06
@bdraco bdraco requested review from asvetlov and webknjaz as code owners April 9, 2025 04:06
Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@bdraco
Copy link
Member Author

bdraco commented Apr 9, 2025

Thanks for the reviews.

0100 here so I'll get these merged and a release done tomorrow after some 💤

@bdraco bdraco merged commit 78761cf into master Apr 9, 2025
111 of 115 checks passed
@bdraco bdraco deleted the leak_ci_arg_to_key branch April 9, 2025 17:25
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.

Memory leak creating istr in multidict 6.3.0,6.3.1,6.3.2 (fixed in 6.4.0)

2 participants