gh-151722: Defer GC tracking of frozendict to end of construction#151740
Open
corona10 wants to merge 5 commits into
Open
gh-151722: Defer GC tracking of frozendict to end of construction#151740corona10 wants to merge 5 commits into
corona10 wants to merge 5 commits into
Conversation
Member
Author
TSAN scriptimport threading, gc
N = 4000
stop = threading.Event()
box = []
def reader():
while not stop.is_set():
if box:
o = box[0]
try:
len(o); hash(o); repr(o)
except Exception:
pass
class Evil:
def __init__(self):
self.n = 0
def keys(self):
return [f"k{i}" for i in range(N)]
def __getitem__(self, k):
self.n += 1
if self.n == 10 and not box: # one scan/construction; k0..k8 present
for o in gc.get_objects():
if type(o) is frozendict and "k0" in o and len(o) < N:
box.append(o)
break
return 1
t = threading.Thread(target=reader, daemon=True)
t.start()
for _ in range(40):
box.clear()
frozendict(Evil())
stop.set()
t.join(timeout=2)
print("observed half-built:", bool(box))AS-ISTO-BE |
Member
Author
|
cc @tonghuaroot |
Contributor
|
Thanks for the cc. Built the PR ( Two sibling creation paths the defer-track doesn't reach yet, both pre-existing + TSan-confirmed on this branch:
Also no committed test yet — the fromkeys one above doubles as a |
methane
reviewed
Jun 20, 2026
| if (self == NULL) { | ||
| return NULL; | ||
| } | ||
| if (!_PyObject_GC_IS_TRACKED(self)) { |
Member
Author
There was a problem hiding this comment.
I think we don't need it
Member
Author
|
markshannon
reviewed
Jun 20, 2026
| } | ||
|
|
||
| static PyObject * | ||
| dict_new(PyTypeObject *type, PyObject *args, PyObject *kwds) |
Member
There was a problem hiding this comment.
This function is strange. Why does it ignore its parameters?
Creating a dict with arguments should create a dict from those arguments.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Member
Author
There was a problem hiding this comment.
Lines 5245 to 5269 in aa5b164
The function already ignore those params, and it was done by dict_init
Member
There was a problem hiding this comment.
Indeed:
>>> dict.__new__(dict, (), a=1,b=2)
{}but frozendict is inconsistent:
>>> frozendict.__new__(frozendict, (), a=1,b=2)
frozendict({'a': 1, 'b': 2})
Member
Author
There was a problem hiding this comment.
I think that than we need to modify frozenset also
>>> set.__new__(set, [1,2,3])
set()
>>> frozenset.__new__(frozenset, [1,2,3])
frozenset({1, 2, 3})
Let's handle this at separate issue.
Member
|
See #151722 (comment) for the proper fix for the issue. Having said that, deferring tracking of any object until the object is complete is a good idea. |
Member
Author
|
@markshannon @methane I've updated the PR. PTAL :) |
corona10
commented
Jun 20, 2026
| STORE_USED(mp, other->ma_used); | ||
| ASSERT_CONSISTENT(mp); | ||
|
|
||
| if (_PyObject_GC_IS_TRACKED(other) && !_PyObject_GC_IS_TRACKED(mp)) { |
Member
Author
There was a problem hiding this comment.
No more lazy track from here, we can remove this logic safely.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.