gh-111968: Use per-thread freelists for dict in free-threading #114323

corona10 · 2024-01-19T16:45:02Z

Issue: Use per-thread freelists in --disable-gil builds #111968

colesbury

I think we will want to keep the free-list state separate from the non-free-list related per-interpreter dictionary state (like watchers, and keys versions). I think the behavior would be incorrect if this were per-thread state.

Include/internal/pycore_freelist.h

Objects/dictobject.c

corona10 · 2024-01-19T17:36:38Z

Thank you for catching, there was some mistake that I didn't catch the issue :(, I will update the soon.

Co-authored-by: Sam Gross <[email protected]>

Include/internal/pycore_interp.h

colesbury

I have a suggestion about the naming of "global_dict_state", but otherwise LGTM.

colesbury · 2024-01-19T18:27:59Z

Include/internal/pycore_global_dict_state.h

 #define DICT_MAX_WATCHERS 8

-struct _Py_dict_state {
+struct _Py_global_dict_state {


Maybe _Py_dict_interp_state?

In some places we use "global_state" to mean runtime-global (not interpreter). For example, we have:

_dtoa_interp_state (per-interpreter)

_mimalloc_interp_state (per-interpreter)

_obmalloc_global_state (in PyRuntime)

corona10 · 2024-01-20T00:35:38Z

Include/internal/pycore_interp.h

@@ -41,6 +41,22 @@ struct _Py_long_state {
    int max_str_digits;
 };

+#define DICT_MAX_WATCHERS 8
+
+struct _Py_dict_interp_state {


@colesbury
More proper position might be pycore_dict.h but there are some header dependency issue.
So let's declare the _Py_dict_interp_state at here.

Yeah, pycore_dict.h (or pycore_dict_state.h) seem like a better locations, but if there are technical reasons that is difficult, this does not seem like such a big deal either.

The only real change is pulling the freelist out. Why can't the remainder stay in pycore_dict_state,h?

It's still possible, I guess :) I will revert about it.

corona10 · 2024-01-30T00:30:40Z

@colesbury SInce I resolved the conflict, Can you please take a look again? cc @ericsnowcurrently

colesbury

This still looks good to me. As I understand it, the plan is to address Eric's three remaining comments in subsequent PRs.

ericsnowcurrently · 2024-02-01T19:09:17Z

I thought "revert the PyDict_Fini() signature" was going to be done in this PR. I'm fine with doing it in a separate PR if that makes sense.

corona10 · 2024-02-01T19:58:06Z

I thought "revert the PyDict_Fini() signature" was going to be done in this PR. I'm fine with doing it in a separate PR if that makes sense.

@ericsnowcurrently
4a76399
Done! :) I will merge the PR!

Objects/dictobject.c

bedevere-app · 2024-02-01T20:05:25Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

ericsnowcurrently · 2024-02-01T20:06:04Z

You already fixed it. 😄

corona10 · 2024-02-01T20:07:42Z

You already fixed it. 😄

But I like your suggestion :) That was more closer to my original intention, I updated it.

corona10 · 2024-02-01T20:08:36Z

I have made the requested changes; please review again

ericsnowcurrently · 2024-02-01T20:27:17Z

FYI, I left a comment on the issue enumerating the things we punted on in this PR: #111968 (comment).

ericsnowcurrently

LGTM

corona10 · 2024-02-01T20:33:10Z

FYI, I left a comment on the issue enumerating the things we punted on in this PR: #111968 (comment).

Thanks!

…ythongh-114323)

pythongh-111968: Use per-thread freelists for dict in free-threading

8561efc

corona10 requested a review from colesbury January 19, 2024 16:45

corona10 requested review from a team, methane and markshannon as code owners January 19, 2024 16:45

bedevere-app bot added the awaiting core review label Jan 19, 2024

bedevere-app bot mentioned this pull request Jan 19, 2024

Use per-thread freelists in --disable-gil builds #111968

Closed

corona10 added skip news topic-free-threading and removed awaiting core review labels Jan 19, 2024

colesbury requested changes Jan 19, 2024

View reviewed changes

Include/internal/pycore_freelist.h Outdated Show resolved Hide resolved

Include/internal/pycore_freelist.h Outdated Show resolved Hide resolved

Objects/dictobject.c Outdated Show resolved Hide resolved

bedevere-app bot added the awaiting core review label Jan 19, 2024

corona10 changed the title ~~gh-111968: Use per-thread freelists for dict in free-threading~~ [WIP] gh-111968: Use per-thread freelists for dict in free-threading Jan 19, 2024

corona10 added the DO-NOT-MERGE label Jan 19, 2024

Address code review

6a69d6f

corona10 removed the DO-NOT-MERGE label Jan 19, 2024

Update Objects/dictobject.c

b2d4606

Co-authored-by: Sam Gross <[email protected]>

corona10 changed the title ~~[WIP] gh-111968: Use per-thread freelists for dict in free-threading~~ gh-111968: Use per-thread freelists for dict in free-threading Jan 19, 2024

corona10 requested a review from colesbury January 19, 2024 17:48

corona10 commented Jan 19, 2024

View reviewed changes

Include/internal/pycore_interp.h Show resolved Hide resolved

corona10 added 3 commits January 20, 2024 02:59

nit

8889072

Remove duplicated DICT_MAX_WATCHERS

0cecf66

nit

7041d4a

colesbury approved these changes Jan 19, 2024

View reviewed changes

Address code review

e973352

corona10 changed the title ~~gh-111968: Use per-thread freelists for dict in free-threading~~ [WIP] gh-111968: Use per-thread freelists for dict in free-threading Jan 20, 2024

Remove unnecessary changes

514a66f

corona10 changed the title ~~[WIP] gh-111968: Use per-thread freelists for dict in free-threading~~ gh-111968: Use per-thread freelists for dict in free-threading Jan 20, 2024

corona10 commented Jan 20, 2024

View reviewed changes

corona10 added 2 commits January 30, 2024 09:13

Merge remote-tracking branch 'upstream/main' into pythongh-111968-dict

9eff9a9

nit

e1d9549

colesbury approved these changes Jan 30, 2024

View reviewed changes

methane approved these changes Jan 30, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Jan 30, 2024

Revert _PyDict_Fini

4a76399

fix

11f81d1

ericsnowcurrently requested changes Feb 1, 2024

View reviewed changes

Objects/dictobject.c Outdated Show resolved Hide resolved

Objects/dictobject.c Outdated Show resolved Hide resolved

bedevere-app bot added awaiting changes and removed awaiting merge labels Feb 1, 2024

Address code review

86e3228

ericsnowcurrently approved these changes Feb 1, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Feb 1, 2024

fix

42a3541

corona10 enabled auto-merge (squash) February 1, 2024 20:32

corona10 merged commit 1390796 into python:main Feb 1, 2024
32 checks passed

bedevere-app bot removed the awaiting merge label Feb 1, 2024

corona10 deleted the gh-111968-dict branch February 1, 2024 21:01

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-111968: Use per-thread freelists for dict in free-threading (p…

17f1760

…ythongh-114323)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-111968: Use per-thread freelists for dict in free-threading #114323

gh-111968: Use per-thread freelists for dict in free-threading #114323

corona10 commented Jan 19, 2024 •

edited by bedevere-app bot

Loading

colesbury left a comment

corona10 commented Jan 19, 2024

colesbury left a comment

colesbury Jan 19, 2024 •

edited

Loading

corona10 Jan 20, 2024

colesbury Jan 22, 2024

ericsnowcurrently Jan 26, 2024

corona10 Jan 26, 2024

ericsnowcurrently Jan 26, 2024

corona10 commented Jan 30, 2024 •

edited

Loading

colesbury left a comment

ericsnowcurrently commented Feb 1, 2024

corona10 commented Feb 1, 2024 •

edited

Loading

bedevere-app bot commented Feb 1, 2024

ericsnowcurrently commented Feb 1, 2024

corona10 commented Feb 1, 2024

corona10 commented Feb 1, 2024

ericsnowcurrently commented Feb 1, 2024

ericsnowcurrently left a comment

corona10 commented Feb 1, 2024

gh-111968: Use per-thread freelists for dict in free-threading #114323

gh-111968: Use per-thread freelists for dict in free-threading #114323

Conversation

corona10 commented Jan 19, 2024 • edited by bedevere-app bot Loading

colesbury left a comment

Choose a reason for hiding this comment

corona10 commented Jan 19, 2024

colesbury left a comment

Choose a reason for hiding this comment

colesbury Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

corona10 Jan 20, 2024

Choose a reason for hiding this comment

colesbury Jan 22, 2024

Choose a reason for hiding this comment

ericsnowcurrently Jan 26, 2024

Choose a reason for hiding this comment

corona10 Jan 26, 2024

Choose a reason for hiding this comment

ericsnowcurrently Jan 26, 2024

Choose a reason for hiding this comment

corona10 commented Jan 30, 2024 • edited Loading

colesbury left a comment

Choose a reason for hiding this comment

ericsnowcurrently commented Feb 1, 2024

corona10 commented Feb 1, 2024 • edited Loading

bedevere-app bot commented Feb 1, 2024

ericsnowcurrently commented Feb 1, 2024

corona10 commented Feb 1, 2024

corona10 commented Feb 1, 2024

ericsnowcurrently commented Feb 1, 2024

ericsnowcurrently left a comment

Choose a reason for hiding this comment

corona10 commented Feb 1, 2024

corona10 commented Jan 19, 2024 •

edited by bedevere-app bot

Loading

colesbury Jan 19, 2024 •

edited

Loading

corona10 commented Jan 30, 2024 •

edited

Loading

corona10 commented Feb 1, 2024 •

edited

Loading