GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-112075: Use PyMem_* for allocating dict keys objects by DinoV · Pull Request #114543 · python/cpython

Conversation

@DinoV

@DinoV DinoV commented Jan 25, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Use PyMem_* for allocating dict keys objects

Currently dict keys are allocated using PyObject_Malloc. This isn't right for free-threaded builds because we don't want non-object allocations to overlap with the object heap. Doing so breaks the guarantees that we can apply an incref to a value which may have been freed and back it out if we lost the reference to the logic. This just switches the allocations to use PyMem_* instead.

This also eliminates some duplication by moving the decref into dictkeys_decref when the ref count drops to zero. This is the only place we need to do this, but the logic to move to the free list or free is currently duplicated. This will become a little more complex with QSBR so we may as well isolate it all to one spot.

  • Issue: Make dict objects thread-safe in --disable-gil builds #112075

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot mentioned this pull request Jan 25, 2024
Make dict objects thread-safe in --disable-gil builds #112075
Closed
@DinoV DinoV added skip news topic-free-threading labels Jan 25, 2024
@DinoV
DinoV force-pushed the nogil_dict_keys_alloc branch 2 times, most recently from f7a9f27 to e5b172b Compare January 25, 2024 20:16
@DinoV
DinoV requested a review from colesbury January 25, 2024 20:42
@DinoV
DinoV marked this pull request as ready for review January 25, 2024 20:58
@DinoV
DinoV requested review from markshannon and methane as code owners January 25, 2024 20:58
@bedevere-app bedevere-app Bot added the awaiting core review label Jan 25, 2024
colesbury
colesbury reviewed Jan 25, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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.

The comment on line 1546 should be updated, but otherwise lgtm

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Objects/dictobject.c Resolved

Uh oh!

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

Comment thread Objects/dictobject.c
}
#endif
PyObject_Free(keys);
PyMem_Free(keys);

@colesbury colesbury Jan 25, 2024

Copy link
Copy Markdown
Contributor

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.

FYI, @corona10 this might cause merge conflicts with #114323, but I don't think they'll be too difficult to resolve.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Objects/dictobject.c Resolved

Uh oh!

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

@DinoV
DinoV force-pushed the nogil_dict_keys_alloc branch from e5b172b to 4a5a20d Compare January 26, 2024 17:53
@DinoV
Use PyMem_* for keys allocation
baaf99d
@DinoV
DinoV force-pushed the nogil_dict_keys_alloc branch from 4a5a20d to baaf99d Compare January 27, 2024 01:19
colesbury
colesbury approved these changes Jan 27, 2024
View reviewed changes
methane
methane approved these changes Jan 27, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jan 27, 2024
@DinoV
DinoV merged commit 3d71665 into python:main Jan 29, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 29, 2024
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@DinoV @aisk
pythongh-112075: Use PyMem_* for allocating dict keys objects (python…
602fc65
…#114543)

Use PyMem_* for keys allocation
@DinoV
DinoV deleted the nogil_dict_keys_alloc branch May 31, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methane methane methane approved these changes
@colesbury colesbury colesbury approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

skip news topic-free-threading

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.

3 participants

@DinoV @methane @colesbury
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.