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-117657: use relaxed loads for checking dict keys immortality #118067

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

DinoV
Copy link
Contributor

@DinoV DinoV commented Apr 18, 2024

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

@DinoV DinoV requested review from mpage and colesbury 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
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.

LGTM

@@ -441,7 +441,7 @@ static void free_keys_object(PyDictKeysObject *keys, bool use_qsbr);
static inline void
dictkeys_incref(PyDictKeysObject *dk)
{
if (dk->dk_refcnt == _Py_IMMORTAL_REFCNT) {
if (FT_ATOMIC_LOAD_SSIZE_RELAXED(dk->dk_refcnt) == _Py_IMMORTAL_REFCNT) {
Copy link
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
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
39 checks passed
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 RHEL7 LTO 3.x has failed when building commit 1e4a4c4.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/507/builds/7247) and take a look at the build logs.
  4. Check if the failure is related to this commit (1e4a4c4) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/507/builds/7247

Failed tests:

  • test_capi

Summary of the results of the build (if available):

==

Click to see traceback logs
remote: Enumerating objects: 7, done.        
remote: Counting objects:  16% (1/6)        
remote: Counting objects:  33% (2/6)        
remote: Counting objects:  50% (3/6)        
remote: Counting objects:  66% (4/6)        
remote: Counting objects:  83% (5/6)        
remote: Counting objects: 100% (6/6)        
remote: Counting objects: 100% (6/6), done.        
remote: Compressing objects:  16% (1/6)        
remote: Compressing objects:  33% (2/6)        
remote: Compressing objects:  50% (3/6)        
remote: Compressing objects:  66% (4/6)        
remote: Compressing objects:  83% (5/6)        
remote: Compressing objects: 100% (6/6)        
remote: Compressing objects: 100% (6/6), done.        
remote: Total 7 (delta 0), reused 2 (delta 0), pack-reused 1        
From https://github.com/python/cpython
 * branch            main       -> FETCH_HEAD
Note: checking out '1e4a4c4897d0f45b1f594bc429284c82efe49188'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b new_branch_name

HEAD is now at 1e4a4c4... gh-117657: use relaxed loads for checking dict keys immortality (#118067)
Switched to and reset branch 'main'

make: *** [Makefile:2229: buildbottest] Error 2

@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