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-99113: A Per-Interpreter GIL! #99114

Closed

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Nov 5, 2022

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 from Py_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:

  • PEP 684 is accepted (if it is accepted), which might be up to a few months due to the changing steering council
  • interpreters have been sufficiently isolated (see my checklist)
  • we have been extra careful about testing this

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.

@gvanrossum
Copy link
Member

318 commits, 114 files, 23 reviewers… How are we going to do this?

@arhadthedev
Copy link
Member

arhadthedev commented May 5, 2023

318 commits, 114 files, 23 reviewers…

We can create a separate PR that just defines a no-op slot Py_mod_multiple_interpreters/Py_MOD_* and adds {Py_mod_multiple_interpreters, Py_MOD_PER_INTERPRETER_GIL_SUPPORTED} to the modules.

Thanks to it, this PR will become a much smaller follow-up.

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood removed request for https://github.com/orgs/python/teams/windows-team, corona10, pablogsal, erlend-aasland, Fidget-Spinner, isidentical, kumaraditya303 and AlexWaygood

Not sure how that happened — I only meant to remove my own request for review...

@erlend-aasland
Copy link
Contributor

318 commits, 114 files, 23 reviewers… How are we going to do this?

Actually, most of the changed files are repetitive, trivial updates of various extension module's PyModuleDef structs.

@arhadthedev
Copy link
Member

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
Copy link
Member

zooba commented May 5, 2023

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.

image

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;
Copy link
Member

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?

Copy link
Member Author

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;
Copy link
Member

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.

Copy link
Member Author

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.

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've left a TODO note on the issue: #99113 (comment).

@ericsnowcurrently
Copy link
Member Author

I had seriously considered splitting this PR up yesterday. I should have listened to myself. 🙂 I'm going to split it up.

@ericsnowcurrently
Copy link
Member Author

I've noted the new PRs above in the PR summary.

ericsnowcurrently added a commit that referenced this pull request May 5, 2023
…04148)

I'll be adding a value to indicate support for per-interpreter GIL in gh-99114.
jbower-fb pushed a commit to jbower-fb/cpython-jbowerfb that referenced this pull request May 8, 2023
…pythongh-104148)

I'll be adding a value to indicate support for per-interpreter GIL in pythongh-99114.
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.

9 participants