| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
The TSAN nogil build fails tests for me on the main 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
|
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| PyInterpreterState *main_interp = _PyInterpreterState_Main(); | ||
| if (interp != main_interp && | ||
| interp->feature_flags & Py_RTFLAGS_USE_MAIN_OBMALLOC) { |
There was a problem hiding this comment.
FYI, you can also use _Py_IsMainInterpreter():
| 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) { |
Sorry, something went wrong.
The case with shared objects (other than interned strings) can't seem to happen so comment that code out. Small code cleanups. Simplify the _testembed.c test, don't need a separate helper module.
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 the m_copy dict stored in runtime structure. So, maybe we are safe from that one.
Fixed as you suggest. |
Sorry, something went wrong.
|
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 using feature_flags but I think that's not the correct place to put the flag. It's not a public API. |
Sorry, something went wrong.
|
I wonder: for interpreters that use_main_obmalloc, would it make sense to also use the main ref chain? |
Sorry, something went wrong.
I think that could work and would simplify the code. The major impact would be that gc.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? |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Closing since GH-125709 fixes the same issue and seems a better approach. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Fix the TraceRefs build to handle interned strings (both mortal and immortal) that are shared between interpreters.