Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-125286: Bug fix for trace-refs logic and shared objects#125314

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

@nascheme
Copy link
Member

@naschemenascheme commentedOct 11, 2024
edited
Loading

Fix the TraceRefs build to handle interned strings (both mortal and immortal) that are shared between interpreters.

@nascheme
Copy link
MemberAuthor

The TSAN nogil build fails tests for me on themain branch as well. So I'm not sure this change is the cause of the test failure. I get these failures on main branch:

8 tests failed:    test_capi.test_mem test_capi.test_pyatomic test_functools    test_importlib test_queue test_signal test_threading_local    test_threadsignals

Copy link
Member

@ericsnowcurrentlyericsnowcurrently left a comment

Choose a reason for hiding this comment

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

I'm a little unclear on the change here. My understanding is that we don't want the interned strings on legacy subinterpreters to be tracked on those interpreter's refs. It seems to me like the change in this PR has stopped fixing that for each interpreter, which would put us back to where we were before. What am I missing?

Other than that, I have one small suggestion.

Comment on lines 2539 to 2541
PyInterpreterState*main_interp=_PyInterpreterState_Main();
if (interp!=main_interp&&
interp->feature_flags&Py_RTFLAGS_USE_MAIN_OBMALLOC) {

Choose a reason for hiding this comment

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

FYI, you can also use_Py_IsMainInterpreter():

Suggested change
PyInterpreterState*main_interp=_PyInterpreterState_Main();
if (interp!=main_interp&&
interp->feature_flags&Py_RTFLAGS_USE_MAIN_OBMALLOC) {
if (_Py_IsMainInterpreter(interp)&&
interp->feature_flags&Py_RTFLAGS_USE_MAIN_OBMALLOC) {

The case with shared objects (other than interned strings) can't seem tohappen so comment that code out.  Small code cleanups.  Simplify the_testembed.c test, don't need a separate helper module.
@nascheme
Copy link
MemberAuthor

nascheme commentedOct 13, 2024
edited
Loading

I'm a little unclear on the change here. My understanding is that we don't want the interned strings on legacy subinterpreters to be tracked on those interpreter's refs. It seems to me like the change in this PR has stopped fixing that for each interpreter, which would put us back to where we were before. What am I missing?

A problem with trace refs occurs both with immortal and mortal interned strings. The previous fix,_Py_NormalizeImmortalReference, only handled the immortal case. The new test I added triggers both cases and if you remove either fix, it crashes in_Py_ForgetReference.

I was trying to trigger a third case, where objects other than interned strings are shared and are created in one interpreter and deallocated in a different one. That would cause_Py_ForgetReference to fail too. However, I can't seem to make it happen, I think because of them_copy dict stored in runtime structure. So, maybe we are safe from that one.

Other than that, I have one small suggestion.

Fixed as you suggest.

@nascheme
Copy link
MemberAuthor

nascheme commentedOct 13, 2024
edited
Loading

Maybe I overdid it with the comments, some are a bit redundant. Another idea I was toying with is to make this tracerefs cleanup logic active only if non-isolated interpreters are being used. You need a flag on the main interp to know if sub-interpreters have been used that share its interned string dict. Possible patch here:

https://gist.github.com/nascheme/2243a137b9f9aa4639590231e3b8e004

It adds a new set of internal flags to the interp state so I think it can't be backported since it changes the ABI. I tried usingfeature_flags but I think that's not the correct place to put the flag. It's not a public API.

@encukou
Copy link
Member

I wonder: for interpreters thatuse_main_obmalloc, would it make sense to also use the main ref chain?

ericsnowcurrently reacted with thumbs up emoji

@nascheme
Copy link
MemberAuthor

I wonder: for interpreters thatuse_main_obmalloc, would it make sense to also use the main ref chain?

I think that could work and would simplify the code. The major impact would be thatgc.getobjects() in one of those interpreters (could be main) would return objects for all sub-interpreters with that flag. Maybe that's an improvement actually. I'm not sure what people would expect. Ask@pablogsal maybe since he uses the TraceRefs build?

@pablogsal
Copy link
Member

I think that the assumption so far of that call is to get all objects in existence. I don't think semantics have been defined after subinterpeters were introduced so we have some wiggle room here

@ericsnowcurrently
Copy link
Member

Here's a PR that shares the main refchain with all legacy interpreters:#125709. FWIW, they all used to share it when it was stored in global variables.

encukou reacted with heart emoji

@nascheme
Copy link
MemberAuthor

Closing sinceGH-125709 fixes the same issue and seems a better approach.

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

Reviewers

@ericsnowcurrentlyericsnowcurrentlyericsnowcurrently left review comments

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nascheme@encukou@pablogsal@ericsnowcurrently

[8]ページ先頭

©2009-2025 Movatter.jp