Skip to content

[WIP] gh-151722: Defer GC tracking of frozendict.fromkeys() as possible - #152021

Closed
corona10 wants to merge 4 commits into
python:mainfrom
corona10:gh-151722-from-keys
Closed

[WIP] gh-151722: Defer GC tracking of frozendict.fromkeys() as possible#152021
corona10 wants to merge 4 commits into
python:mainfrom
corona10:gh-151722-from-keys

Conversation

@corona10

@corona10corona10 commented Jun 23, 2026

Copy link
Copy Markdown
Member

Co-authored-by: tonghuaroot <tonghuaroot@gmail.com>
@corona10

Copy link
Copy Markdown
MemberAuthor

cc @tonghuaroot (I add you as the co-author), too many things need to be addressed from #151967

Comment threadObjects/dictobject.c
int status;

PyTypeObject *cls_type = _PyType_CAST(cls);
d = _PyObject_CallNoArgs(cls);

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

If there is a good way to obtain an untracked object here, that would be great, but I think that is a separate topic.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is these cases considered?

  • subclass of frozendict implements __init__ instead of __new__, passes its self to another thread
  • another thread obtains a reference to the instance via GC while the __init__ is being executed

Comment threadObjects/dictobject.c
done:
// Built untracked above; GC-track now that it is complete.
if (d != NULL) {
assert(!_PyObject_GC_IS_TRACKED(d));

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@tonghuaroot
By this, we don't need additional test code.

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.

Agreed, the assert covers it. Thanks for taking this over.

@corona10corona10 changed the title gh-151722: Defer GC tracking of frozendict.fromkeys() as possible[WIP] gh-151722: Defer GC tracking of frozendict.fromkeys() as possibleJun 23, 2026
Comment threadObjects/dictobject.c Outdated
}
// The constructor returns a tracked object; keep it untracked while it is
// filled and GC-track it once complete.
_PyObject_GC_UNTRACK(d);

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.

Thanks for driving this. I believe the test_fromkeys CI failure originates here: for an empty exact frozendict, cls() already returns an untracked object (thanks to your gh-151740 defer-track), so this unconditional _PyObject_GC_UNTRACK trips its internal IS_TRACKED assertion. Guarding it with if (_PyObject_GC_IS_TRACKED(d)) should resolve it. Please feel free to adjust as you see fit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tl; dr I wrote PR gh-152067 which is a similar fix, but with a safer design.

I don't think that calling _PyObject_GC_UNTRACK(d) on any type is a good approach. We need to track again the dictionary in many code paths which is very tricky. For example, if dict_dict_fromkeys() fails (return NULL), we need to track again the dictionary... but we no longer have a (safe) reference to d since dict_dict_fromkeys() called Py_DECREF(d). Having to track again the dictionary on error is really complicated to implement.

Moreover, we cannot use Py_INCREF(d) at the beginning (to keep a safe reference to the dictionary) since assert(can_modify_dict(mp)); fails in this case on frozendict.

IMO we should only be worried about the dictionary being not tracked by the GC if the dictionary is a frozendict. And it's a frozendict, we can copy it using frozendict_new_untracked() to create a copy which is not tracked by the GC.

@vstinner

Copy link
Copy Markdown
Member

I merged #152067 instead.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting core reviewneeds backport to 3.15pre-release feature fixes, bugs and security fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@corona10@vstinner@methane@tonghuaroot