Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-99113: A Per-Interpreter GIL!#99114
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
gh-99113: A Per-Interpreter GIL!#99114
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This reverts commit25fd52a.
318 commits, 114 files, 23 reviewers… How are we going to do this? |
arhadthedev commentedMay 5, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We can create a separate PR that just defines a no-op slot Thanks to it, this PR will become a much smaller follow-up. |
Not sure how that happened — I only meant to remove my own request for review... |
Actually, most of the changed files are repetitive, trivial updates of various extension module's PyModuleDef structs. |
Their repetitiveness takes attention from other parts of the diff and requires to constantly scroll through all of them for each inter-file comparison in other parts of the PR. It makes the analysis harder. That's why I proposed to move them out into a separate PR, merge it and update this PR. |
zooba commentedMay 5, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I quickly went through and marked all those files as "Viewed", then used the File Filter drop down to hide viewed files. So I've only got 7 files left with non-trivial changes, one of which is the NEWS entry.
Looks like the "viewed" state is remembered, but the filter needs to be set each time I go back in. |
| const PyInterpreterConfig config = _PyInterpreterConfig_LEGACY_INIT; | ||
| PyInterpreterConfig config = _PyInterpreterConfig_LEGACY_INIT; | ||
| // The main interpreter always has its own GIL. | ||
| config.own_gil = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Presumably we're in a lot of trouble if the main interpreter exits first...
Do we have mechanisms to ensure it outlives any subinterpreters that share its GIL? Or perhaps we should reference count the GIL instead of marking ownership?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, the main interpreter must be the last one. There are many things that break if that isn't the case.
| struct _gil_runtime_state *gil = &_PyRuntime.ceval.gil; | ||
| PyInterpreterState *interp = _PyInterpreterState_Get(); | ||
| struct _gil_runtime_state *gil = interp->ceval.gil; | ||
| assert(gil != NULL); | ||
| gil->interval = microseconds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need some way to propagate this to new subinterpreters? I don't see it if it's here.
Not sure whether this would catch people out or not. Presumably increasing the switch count is less important with proper parallelism, and it's documented as setting "the interpreter's switch interval" so we're clear on that front. But it may be surprising.
Probably only worth adding a note to the sys docs to clarify that it doesn't propagate to new subinterpreter GILs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, we'll make sure the docs are clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've left a TODO note on the issue:#99113 (comment).
I had seriously considered splitting this PR up yesterday. I should have listened to myself. 🙂 I'm going to split it up. |
I've noted the new PRs above in the PR summary. |
…pythongh-104148)I'll be adding a value to indicate support for per-interpreter GIL inpythongh-99114.
Uh oh!
There was an error while loading.Please reload this page.
I've split up this PR:
That last one is effectively the superseder of this one.
This is the culmination of PEP 684 (and of my 8-year long multi-core Python project)!
Each subinterpreter may now be created with its own GIL (via
Py_NewInterpreterFromConfig()). If not so configured then the interpreter will share with the main interpreter--the status quo since the subinterpreters were added decades ago. The main interpreter always has its own GIL and subinterpreters fromPy_NewInterpreter()will always share with the main interpreter.This is essentially the correct implementation but it may change here and there before we've reached the end.
We won't merge this until:
I'm merging in other branches that this one relies on, but those will wash out as the other PRs get merged. In the meantime, you can see the actual changes here:https://github.com/python/cpython/compare/main...ericsnowcurrently:per-interpreter-gil-new-bare?expand=1.