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-101659: Isolate "obmalloc" State to Each Interpreter #101660

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Feb 7, 2023

This is strictly about moving the "obmalloc" runtime state from _PyRuntimeState to PyInterpreterState. Doing so improves isolation between interpreters, specifically most of the memory (incl. objects) allocated for each interpreter's use. This is important for a per-interpreter GIL, but such isolation is valuable even without it.

Benchmarking indicates the performance impact is negligible. (I'll re-run soon to double-check.)

@ericsnowcurrently
Copy link
Member Author

ericsnowcurrently commented Feb 9, 2023

FYI, the current CI failures are due to existing code. Basically, that code (in _PyImport_FixupExtensionObject()) tries to delete a [currently] global object that was created by a different interpreter than the "current" one. (There may be a few other similarly problematic global objects. I'll have to check.) As to why only some of the jobs are failing: the failure is a check by the debug allocator, which is only used in Py_DEBUG builds.

FWIW, the failure is exactly what we should expect to happen when per-interpreter data breaks isolation. I actually spent a while trying to figure out what I was doing wrong in my branch before realizing that everything was working as it should. 😄

UPDATE: I've opened gh-101758 to address this.

@ericsnowcurrently
Copy link
Member Author

Given what I've determined via gh-101758, I'll probably need to make non-isolated interpreters share the obmalloc state with the main interpreter.

@ericsnowcurrently
Copy link
Member Author

I'm tabling this until we've isolated all non-static objects.

@ericsnowcurrently ericsnowcurrently marked this pull request as draft February 27, 2023 16:33
@ericsnowcurrently
Copy link
Member Author

FYI, as with the per-interpreter ref total, I'm probably going to add some code to preserve the per-interpreter allocated blocks when each subinterpreter is destroyed, so any leaks may be handled appropriately when the runtime is finalized. I may also report the per-interpreter count at interpreter finalization. We'll see.

ericsnowcurrently added a commit that referenced this pull request Mar 14, 2023
…h-102661)

It doesn't make sense to use multi-phase init for these modules. Using a per-interpreter "m_copy" (instead of PyModuleDef.m_base.m_copy) makes this work okay. (This came up while working on gh-101660.)

Note that we might instead end up disallowing re-load for sys/builtins since they are so special.

#102660
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.

None yet

3 participants