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

gh-112075: Use PyMem_* for allocating dict keys objects #114543

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

DinoV
Copy link
Contributor

@DinoV DinoV commented Jan 25, 2024

Use PyMem_* for allocating dict keys objects

Currently dict keys are allocated using PyObject_Malloc. This isn't right for free-threaded builds because we don't want non-object allocations to overlap with the object heap. Doing so breaks the guarantees that we can apply an incref to a value which may have been freed and back it out if we lost the reference to the logic. This just switches the allocations to use PyMem_* instead.

This also eliminates some duplication by moving the decref into dictkeys_decref when the ref count drops to zero. This is the only place we need to do this, but the logic to move to the free list or free is currently duplicated. This will become a little more complex with QSBR so we may as well isolate it all to one spot.

@DinoV DinoV force-pushed the nogil_dict_keys_alloc branch 2 times, most recently from f7a9f27 to e5b172b Compare January 25, 2024 20:16
@DinoV DinoV requested a review from colesbury January 25, 2024 20:42
@DinoV DinoV marked this pull request as ready for review January 25, 2024 20:58
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

The comment on line 1546 should be updated, but otherwise lgtm

@@ -697,7 +696,7 @@ free_keys_object(PyInterpreterState *interp, PyDictKeysObject *keys)
return;
}
#endif
PyObject_Free(keys);
PyMem_Free(keys);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, @corona10 this might cause merge conflicts with #114323, but I don't think they'll be too difficult to resolve.

@DinoV DinoV force-pushed the nogil_dict_keys_alloc branch from e5b172b to 4a5a20d Compare January 26, 2024 17:53
@DinoV DinoV force-pushed the nogil_dict_keys_alloc branch from 4a5a20d to baaf99d Compare January 27, 2024 01:19
@DinoV DinoV merged commit 3d71665 into python:main Jan 29, 2024
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@DinoV DinoV deleted the nogil_dict_keys_alloc branch May 31, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants