Skip to content

gh-150490: Raise PyType_Modified for insertion into split dictionary (take 2)#152914

Draft
DinoV wants to merge 1 commit into
python:mainfrom
DinoV:sharedkeys_type2
Draft

gh-150490: Raise PyType_Modified for insertion into split dictionary (take 2)#152914
DinoV wants to merge 1 commit into
python:mainfrom
DinoV:sharedkeys_type2

Conversation

@DinoV

@DinoV DinoV commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Take 2 of #150489 - what's changed is that we now acquire the type lock in insert_split_key before we acquire the keys lock. We never do any work below the dictionary keys lock so we maintain an invariant that we don't have inverted lock acquisition leading to a deadlock. Also adds @nascheme's test case. ./python -m test --tsan-parallel --parallel-threads=4 -j4 -W` is no longer hanging.

When we insert into a split dictionary we update the shared keys version - this is used to invalidate caches for loading methods and loading class values and requires us to check the keys version. Instead we can raise PyType_Modified which lets us rely on the type version check + has inline values check instead of validating that we have the correct keys version. This gets rid of loading the cached keys version, the objects type (although likely the compiler eliminates this already), loading the cached keys from the type, and then loading the keys version and comparing it against the cached value.

@DinoV DinoV force-pushed the sharedkeys_type2 branch from 43d2f71 to e256c6b Compare July 2, 2026 19:47
@nascheme

nascheme commented Jul 2, 2026

Copy link
Copy Markdown
Member

Nice solution! This looks good, with the following minor comments:

  • I think the acquire of types mutex in insert_split_key() should have a comment beside it, explaining the lock ordering that is being relied on. It might not be obvious to people modifying that code in the future.
  • The _PyDict_SplitKeysInvalidated() function doesn't make it clear that it requires that the types mutex is held. I think this could just be a static function, it's only called from dictobject.c
  • Minor point: by switching to calling _PyType_Modified_Unlocked(), we lose the locking skip when tp_version_tag == 0. I don't know if that actually matters, probably not.

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.

2 participants