Skip to content

Don't allocate new memory for htkeys on adding new item if the dict has deleted slots#1200

Merged
asvetlov merged 11 commits intoaio-libs:masterfrom
asvetlov:shrink
Jun 25, 2025
Merged

Don't allocate new memory for htkeys on adding new item if the dict has deleted slots#1200
asvetlov merged 11 commits intoaio-libs:masterfrom
asvetlov:shrink

Conversation

@asvetlov
Copy link
Member

@asvetlov asvetlov commented Jun 25, 2025

It is an optimization that eliminates malloc()/free() calls for the keys structure if it could be collapsed in-place.

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 25, 2025

CodSpeed Performance Report

Merging #1200 will degrade performances by 33.41%

Comparing asvetlov:shrink (11afd08) with master (af83fc3)

Summary

⚡ 4 improvements
❌ 1 (👁 1) regressions
✅ 239 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
test_cimultidict_getall_istr_hit[py] 28.8 ms 20.8 ms +38.5%
test_cimultidict_getall_istr_miss[py] 1.9 ms 1.5 ms +30.01%
test_multidict_getall_str_hit[ci-py] 28.7 ms 20.7 ms +38.61%
test_multidict_getall_str_hit[cs-py] 20.5 ms 16.4 ms +25.3%
👁 test_multidict_getall_str_miss[ci-py] 1.4 ms 2.1 ms -33.41%

@asvetlov asvetlov requested a review from webknjaz as a code owner June 25, 2025 10:51
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jun 25, 2025
asvetlov and others added 2 commits June 25, 2025 14:22
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
asvetlov and others added 2 commits June 25, 2025 14:26
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
@codecov
Copy link

codecov bot commented Jun 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.32%. Comparing base (af83fc3) to head (11afd08).
⚠️ Report is 29 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1200   +/-   ##
=======================================
  Coverage   98.32%   98.32%           
=======================================
  Files          27       27           
  Lines        3874     3874           
  Branches      706      706           
=======================================
  Hits         3809     3809           
  Misses         18       18           
  Partials       47       47           
Flag Coverage Δ
CI-GHA 98.32% <ø> (ø)
MyPy 80.47% <ø> (ø)
pytest 99.85% <ø> (ø)

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.

@asvetlov asvetlov merged commit 8dc666e into aio-libs:master Jun 25, 2025
64 checks passed
@asvetlov asvetlov deleted the shrink branch June 25, 2025 19:32
for (Py_ssize_t i = 0; i < nentries; ++i, ++old_ep) {
if (old_ep->identity != NULL) {
if (new_ep != old_ep) {
*new_ep++ = *old_ep;
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it might be the cause of the segfault

Copy link
Member

Choose a reason for hiding this comment

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

does new_ep always need to be incremented here even if != ?

Copy link
Member

Choose a reason for hiding this comment

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

maybe new_ep == old_ep, first entry to be overwritten by later entries, new_ep now at wrong position, anything after writes to the wrong memory address...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think no.
The loop iterates over all elements and overwrites empty ones with next items.
It basically deletes entries with identity == NULL and collapses unused memory holes

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.

3 participants