GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-99113: A Per-Interpreter GIL! by ericsnowcurrently · Pull Request #99114 · python/cpython

Conversation

ericsnowcurrently commented Nov 5, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

Issue: Implement PEP 684: A Per-Interpreter GIL #99113

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hannes, Mytherin, and CynicDog reacted with thumbs up emoji

iritkatriel, Fidget-Spinner, corona10, dgiger42, bgruening, a-reich, erlend-aasland, ivoflipse, barneygale, gpshead, and 15 more reacted with hooray emoji

All reactions

👍 3 reactions
🎉 25 reactions

eduardo-elizondo added 30 commits

March 8, 2022 16:15


          Revert "Remove unused refcounts in singletons within CPython/Objects"

25fd52a

This reverts commit 5af0167.


          Include immortal interned strings

be86955


          Regen frozen main

38a14a9


          Properly clean up all immortal interned strings at runtime finalization

c828369


          Build and test fixes

ee41af6


          Temporarily disable single test_embed test

f835e6d


          Fix structseq test

8573af4


          Move nonetype refcount to static refcnt

ad19ff6


          Remove unneeded reference counts in Cpython/Objects

66c625f

This reverts commit 25fd52a.


          Mark global instances as static globals

1379d50


          Remove unneeded reference counts in Cpython/Python

1c9ee6d


          Remove unneeded reference counts in gcmodule.c

287b57c


          Cleanup bool and str usage

c736a7c


          Merge branch 'main' into immortal-references

60f0760


          Fix whitespaces

1321ff6


          Remove static immortal bit in favor of unicode intern state

a719b41


          Merge remote-tracking branch 'upstream/main' into immortal-references

9f3ed39


          Regen frozen

8f72afe


          Fix regrtest

52d6d78


          Change immortal refcount for PY_SSIZE_T_MAX

9fd8a98


          Introduce saturated adds for increfs


          Add default and msvc intrinsic saturated add

eb5da8c


          Fix msvc saturated add

def8da3


          Fix docs

fe6727e


          Move unicode_is_singleton to Py_DEBUG

38df3ce


          Skip immortal checks in frame clear

73f6dcd


          Make code objects immortal

d68efa1


          Merge remote-tracking branch 'upstream/main' into immortal-references

18cff33


          Refcount fixes

168a85c


          Temporarily disable two code tests

9ada9fd

ericsnowcurrently requested review from 1st1, asvetlov, kumaraditya303 and willingc as code owners

May 4, 2023 23:59

gvanrossum commented May 5, 2023

Copy link

Copy Markdown

Member

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

TitaniumHocker and zacqed reacted with eyes emoji

All reactions

👀 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

arhadthedev commented May 5, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood removed request for a team, AlexWaygood, Fidget-Spinner, corona10, erlend-aasland, isidentical, kumaraditya303 and pablogsal

May 5, 2023 06:24

AlexWaygood commented May 5, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland commented May 5, 2023

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

arhadthedev commented May 5, 2023

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba commented May 5, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

arhadthedev and erlend-aasland reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba reviewed

May 5, 2023

View reviewed changes

Comment thread

Python/pylifecycle.c

		const PyInterpreterConfig config = _PyInterpreterConfig_LEGACY_INIT;
		PyInterpreterConfig config = _PyInterpreterConfig_LEGACY_INIT;
		// The main interpreter always has its own GIL.
		config.own_gil = 1;

zooba May 5, 2023

Copy link

Copy Markdown

Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently May 5, 2023

Copy link

Copy Markdown

Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/ceval_gil.c

		PyInterpreterState *interp = _PyInterpreterState_Get();
		struct _gil_runtime_state *gil = interp->ceval.gil;
		assert(gil != NULL);
		gil->interval = microseconds;

zooba May 5, 2023

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently May 5, 2023

Copy link

Copy Markdown

Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently May 5, 2023

Copy link

Copy Markdown

Member Author

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently commented May 5, 2023

Copy link

Copy Markdown

Member Author

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

zooba and erlend-aasland reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently closed this

May 5, 2023

This was referenced May 5, 2023

gh-99113: Share the GIL via PyInterpreterState.ceval.gil #104203

Merged

gh-99113: Add PyInterpreterConfig.own_gil #104204

Merged

gh-99113: Make Sure the GIL is Acquired at the Right Places #104208

Merged

gh-99113: A Per-Interpreter GIL! #104209

Closed

gh-99113: A Per-Interpreter GIL! #104210

Merged

ericsnowcurrently commented May 5, 2023

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently added a commit that referenced this pull request

May 5, 2023


          gh-104108: Add the Py_mod_multiple_interpreters Module Def Slot (gh-1…

1c420e1

…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 that referenced this pull request

May 8, 2023


          pythongh-104108: Add the Py_mod_multiple_interpreters Module Def Slot (…

efc84cd

…pythongh-104148)

I'll be adding a value to indicate support for per-interpreter GIL in pythongh-99114.

ericsnowcurrently mentioned this pull request

Jun 12, 2023

Crash During Subinterpreter Finalization #105699

Closed

Reviewers

zooba

zooba left review comments

arhadthedev

arhadthedev left review comments

gvanrossum

Awaiting requested review from gvanrossum

JelleZijlstra

Awaiting requested review from JelleZijlstra

JelleZijlstra is a code owner

rhettinger

Awaiting requested review from rhettinger

rhettinger is a code owner

gpshead

Awaiting requested review from gpshead

gpshead is a code owner

berkerpeksag

Awaiting requested review from berkerpeksag

pganssle

Awaiting requested review from pganssle

pganssle is a code owner

abalkin

Awaiting requested review from abalkin

brettcannon

Awaiting requested review from brettcannon

brettcannon is a code owner

encukou

Awaiting requested review from encukou

ncoghlan

Awaiting requested review from ncoghlan

ncoghlan is a code owner

warsaw

Awaiting requested review from warsaw

warsaw is a code owner

tiran

Awaiting requested review from tiran

1st1

Awaiting requested review from 1st1

1st1 is a code owner

asvetlov

Awaiting requested review from asvetlov

asvetlov is a code owner

willingc

Awaiting requested review from willingc

willingc is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

ericsnowcurrently commented Nov 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented May 5, 2023

Uh oh!

arhadthedev commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood commented May 5, 2023

Uh oh!

erlend-aasland commented May 5, 2023

Uh oh!

arhadthedev commented May 5, 2023

Uh oh!

zooba commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zooba May 5, 2023

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently May 5, 2023

Choose a reason for hiding this comment

Uh oh!

zooba May 5, 2023

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently May 5, 2023

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently May 5, 2023

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently commented May 5, 2023

Uh oh!

ericsnowcurrently commented May 5, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ericsnowcurrently commented Nov 5, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

arhadthedev commented May 5, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

zooba commented May 5, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.