Skip to content

Always return new references in C Extension #1084

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

Closed
wants to merge 4 commits into from
Closed

Conversation

asvetlov
Copy link
Member

Borrowed references to Python objects are dangerous.
The PR constantly uses a new-reference strategy and newer returns a borrowed ref.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Mar 20, 2025
@asvetlov asvetlov marked this pull request as ready for review March 20, 2025 10:45
Copy link

codspeed-hq bot commented Mar 20, 2025

CodSpeed Performance Report

Merging #1084 will degrade performances by 42.42%

Comparing strongrefs (7d5e800) with master (5dcfd85)

Summary

❌ 17 regressions
✅ 36 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
test_cimultidict_extend_istr 129.6 µs 158.6 µs -18.29%
test_cimultidict_fetch_istr 61.7 µs 85.1 µs -27.57%
test_cimultidict_fetch_str 86 µs 109.1 µs -21.14%
test_cimultidict_get_hit 97.4 µs 120.8 µs -19.39%
test_cimultidict_get_hit_with_default 99.4 µs 122.6 µs -18.89%
test_cimultidict_get_istr_hit 73.3 µs 97.2 µs -24.63%
test_cimultidict_get_istr_hit_with_default 75.5 µs 99.3 µs -23.91%
test_cimultidict_get_istr_miss 77.6 µs 129.5 µs -40.06%
test_cimultidict_get_istr_with_default_miss 80.1 µs 132.5 µs -39.55%
test_cimultidict_get_miss 102.6 µs 155.4 µs -33.95%
test_cimultidict_get_miss_with_default 105.1 µs 156.3 µs -32.76%
test_multidict_fetch 56.6 µs 79.6 µs -28.88%
test_multidict_get_hit 61.4 µs 83.1 µs -26.06%
test_multidict_get_miss 66.9 µs 116.2 µs -42.42%
test_keys_view_equals 77.6 µs 90.7 µs -14.46%
test_keys_view_is_disjoint 72.8 µs 97.9 µs -25.64%
test_keys_view_more_or_equal 74.1 µs 86.8 µs -14.65%

@asvetlov
Copy link
Member Author

Hmm, very strange. INCREF/DECREF cannot be so slow!

@asvetlov
Copy link
Member Author

Ok, the approach doesn't work for an item finding.
Internal pair_list.h utilities shouldn't incref/decref

@asvetlov asvetlov closed this Mar 20, 2025
@asvetlov asvetlov deleted the strongrefs branch April 3, 2025 19:29
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