Skip to content

gh-111968: Split _Py_dictkeys_freelist out of _Py_dict_freelist - #115505

Merged
corona10 merged 4 commits into
python:mainfrom
corona10:gh-111968-dict-refactor
Feb 16, 2024
Merged

gh-111968: Split _Py_dictkeys_freelist out of _Py_dict_freelist#115505
corona10 merged 4 commits into
python:mainfrom
corona10:gh-111968-dict-refactor

Conversation

@corona10

@corona10corona10 commented Feb 15, 2024•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

@bedevere-appbedevere-appBot mentioned this pull request Feb 15, 2024
Closed
@corona10corona10 changed the title gh-111968: Split _Py_dictkeys_freelist out of _Py_dict_freelist[WIP[ gh-111968: Split _Py_dictkeys_freelist out of _Py_dict_freelistFeb 15, 2024
@corona10corona10 changed the title [WIP[ gh-111968: Split _Py_dictkeys_freelist out of _Py_dict_freelist[WIP] gh-111968: Split _Py_dictkeys_freelist out of _Py_dict_freelistFeb 15, 2024
@corona10
corona10force-pushed the gh-111968-dict-refactor branch from 20a4955 to 1710e24CompareFebruary 15, 2024 11:59
@corona10corona10 changed the title [WIP] gh-111968: Split _Py_dictkeys_freelist out of _Py_dict_freelistgh-111968: Split _Py_dictkeys_freelist out of _Py_dict_freelistFeb 15, 2024
@corona10

corona10 commented Feb 15, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@ericsnowcurrently@colesbury Ready to review!

And this is the last PR.

colesbury
colesbury reviewed Feb 15, 2024
View reviewed changes
Comment threadInclude/internal/pycore_freelist.h Outdated
/* Dictionary reuse scheme to save calls to malloc and free */
PyDictObject *free_list[PyDict_MAXFREELIST];
int numfree;
int keys_numfree;

@colesburycolesburyFeb 15, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need only one of numfree or keys_numfree in each struct.

@corona10corona10Feb 15, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah copy and paste accident :)

ericsnowcurrently
ericsnowcurrently requested changes Feb 15, 2024
View reviewed changes

@ericsnowcurrentlyericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM

Aside from agreeing with @colesbury's point about dropping the "keys_numfree" fields, I have one mild suggestion about some local variable names (i.e. drop the "dict_"/"dictkeys_" prefix where not needed).

Comment threadInclude/internal/pycore_freelist.h Outdated
Comment threadInclude/internal/pycore_freelist.h Outdated
Comment threadObjects/dictobject.c Outdated
Comment threadObjects/dictobject.c Outdated
Comment threadObjects/dictobject.c Outdated
Comment threadObjects/dictobject.c Outdated
@bedevere-app

bedevere-appBot commented Feb 15, 2024

Copy link
Copy Markdown

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

@ericsnowcurrently

ericsnowcurrently commented Feb 15, 2024

Copy link
Copy Markdown
Member

And thanks for taking the time for this PR!

@corona10

corona10 commented Feb 15, 2024

Copy link
Copy Markdown
MemberAuthor

I have made the requested changes; please review again

ericsnowcurrently
ericsnowcurrently approved these changes Feb 15, 2024
View reviewed changes

@ericsnowcurrentlyericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adjusting that! I have just one other small suggestion.

I'm approving the PR. I'll leave it up to you about my final suggestion.

Comment threadInclude/internal/pycore_freelist.h Outdated
Comment threadObjects/dictobject.c Outdated
@corona10
corona10 requested a review from 1st1 as a code ownerFebruary 16, 2024 00:26
@corona10
corona10 enabled auto-merge (squash) February 16, 2024 00:27
@corona10
corona10 merged commit 321d13f into python:mainFeb 16, 2024
@ericsnowcurrently

ericsnowcurrently commented Feb 16, 2024

Copy link
Copy Markdown
Member

Thanks again for all the great work on the freelists, @corona10!

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@corona10@diegorusso
pythongh-111968: Split _Py_dictkeys_freelist out of _Py_dict_freelist (…
c96dc8f
…pythongh-115505)
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
@corona10@LukasWoodtli
pythongh-111968: Split _Py_dictkeys_freelist out of _Py_dict_freelist (…
6c72605
…pythongh-115505)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrentlyericsnowcurrentlyericsnowcurrently approved these changes
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner
@gvanrossumgvanrossumAwaiting requested review from gvanrossum
@methanemethaneAwaiting requested review from methanemethane is a code owner
@colesburycolesburyAwaiting requested review from colesbury
@1st11st1Awaiting requested review from 1st11st1 is a code owner

Assignees

No one assigned

Labels

skip newstopic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@corona10@ericsnowcurrently@colesbury