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-124785: re-work fix so tracerefs test passes #124808

Closed

Conversation

nascheme
Copy link
Member

@nascheme nascheme commented Sep 30, 2024

The previous fix to this bug caused some trace-refs tests to fail. Object references from the sub-interpreters were not being correctly accounted in the main interpreter. Re-work the fix so that the interned strings for sub-interpreters go into their own dict, interned_strings_legacy. That allows the main interpreter to clean them knowing that those specific strings have been allocated in sub-interpreters.

Comment on lines +304 to +305
return (interp != main_interp &&
!(interp->feature_flags & Py_RTFLAGS_MULTI_INTERP_EXTENSIONS));
Copy link
Member

Choose a reason for hiding this comment

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

FYI, (not Py_RTFLAGS_MULTI_INTERP_EXTENSIONS) does not imply Py_RTFLAGS_USE_MAIN_OBMALLOC. Why not stick with Py_RTFLAGS_USE_MAIN_OBMALLOC?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm looking at reload_singlephase_extension and when we can end up in the PyDict_Update case. I think it can only happen if Py_RTFLAGS_MULTI_INTERP_EXTENSIONS is set. If we do that dict update then we are sharing objects between interpreters.

Objects/unicodeobject.c Outdated Show resolved Hide resolved
Spelling fix.

Co-authored-by: Eric Snow <[email protected]>
@nascheme
Copy link
Member Author

nascheme commented Oct 1, 2024

For the backport to 3.12 and 3.13 we could make interned_strings_legacy a global var rather than part of the interpreter state structure (if that's a problem for maintaining the ABI). I think that's safe because only the main interpreter would use that global.

That's assuming we think this change is appropriate as a backported fix. Another option would be GH-124796, simpler code but it leaks the immortal strings whereas this PR frees them (eventually).

Objects/unicodeobject.c Outdated Show resolved Hide resolved
Objects/unicodeobject.c Outdated Show resolved Hide resolved
@nascheme
Copy link
Member Author

nascheme commented Oct 1, 2024

Closing this. After discussion with Eric Snow, I think his GH-124865 is the preferred fix. It has the advantage that it's just a relatively small change on top of GH-124646 (to fix the tracerefs issue) and it doesn't touch the ABI like this one does. So, it should be easier to backport.

@nascheme nascheme closed this Oct 1, 2024
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