gh-117657: use relaxed loads for checking dict keys immortality by DinoV · Pull Request #118067 · python/cpython · GitHub
Skip to content

gh-117657: use relaxed loads for checking dict keys immortality - #118067

Merged
DinoV merged 1 commit into
python:mainfrom
DinoV:nogil_dict_keys_ref_tsan
Apr 19, 2024
Merged

gh-117657: use relaxed loads for checking dict keys immortality#118067
DinoV merged 1 commit into
python:mainfrom
DinoV:nogil_dict_keys_ref_tsan

Conversation

@DinoV

@DinoV DinoV commented Apr 18, 2024

Copy link
Copy Markdown
Contributor

Checking for immortality of dict keys is reported as a TSAN violation, we just need relaxed loads

@DinoV
DinoV requested review from colesbury and mpage April 18, 2024 22:06
@DinoV DinoV changed the title bpo-117657: use relaxed loads for checking dict keys immortality gh-117657: use relaxed loads for checking dict keys immortality Apr 18, 2024

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread Objects/dictobject.c

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could also do dk == Py_EMPTY_KEYS and avoid the atomics, but this seems fine too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kind of like that, but am a little worried that it'll be overlooked if any other keys become immortal, so I'll just keep it as-is for now.

@DinoV
DinoV marked this pull request as ready for review April 19, 2024 16:24
@DinoV
DinoV merged commit 1e4a4c4 into python:main Apr 19, 2024
@bedevere-bot

Copy link
Copy Markdown

@DinoV
DinoV deleted the nogil_dict_keys_ref_tsan branch May 31, 2024 18:22
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.

3 participants