GeezerVison

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

GeezerVison is showing github.com — gh-112075: Add try-incref functions from nogil branch for use in dict thread safety by DinoV · Pull Request #114512 · python/cpython

Conversation

DinoV commented Jan 24, 2024 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

The NoGIL branch has functions for attempting to do an incref on an object which may or may not be in flight. This just brings those functions over so that they will be usable from in the dict implementation to get items w/o holding a lock.

There's a handful of small simple modifications:

Adding inline to the force inline functions to avoid a warning, and switching from _Py_ALWAYS_INLINE to Py_ALWAYS_INLINE as that's available
Remove _Py_REF_LOCAL_SHIFT as it doesn't exist yet (and is currently 0 in the 3.12 nogil branch anyway)
ob_ref_shared is currently Py_ssize_t and not uint32_t, so use that
_PY_LIKELY doesn't exist, so drop it
_Py_ThreadLocal becomes _Py_IsOwnedByCurrentThread
Add '_PyInterpreterState_GET()' to _Py_IncRefTotal calls.

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 Bot mentioned this pull request

Jan 24, 2024

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

Closed

DinoV added skip news topic-free-threading labels

Jan 24, 2024

DinoV force-pushed the nogil_tryincref branch 4 times, most recently from 9318df9 to 818fc5f Compare

January 24, 2024 18:52

DinoV requested a review from colesbury

January 24, 2024 19:10

DinoV marked this pull request as ready for review

January 24, 2024 19:25

bedevere-app Bot added the awaiting core review label

Jan 24, 2024

colesbury reviewed

Jan 24, 2024

View reviewed changes

Comment thread

Include/internal/pycore_object.h Resolved

Uh oh!

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

Comment thread

Include/internal/pycore_object.h Resolved

Uh oh!

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

Comment thread

Include/internal/pycore_object.h Resolved

Uh oh!

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

Comment thread

Include/internal/pycore_object.h Resolved

Uh oh!

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

Comment thread

Include/internal/pycore_object.h Resolved

Uh oh!

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

Comment thread

Include/internal/pycore_object.h Resolved

Uh oh!

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

Comment thread

Include/internal/pycore_object.h Resolved

Uh oh!

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

Comment thread

Include/internal/pycore_object.h Outdated


		/* Tries to incref the object op and ensures that src still points to it. /
		static inline int
		_Py_TryAcquireObject(PyObject *src, PyObject op)

colesbury Jan 24, 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.

I don't love this name, but I don't know what it should be called.

Sorry, something went wrong.

Uh oh!

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

All reactions

DinoV Jan 24, 2024

Copy link

Copy Markdown

Contributor 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 don't really have a better name either :P

Sorry, something went wrong.

Uh oh!

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

All reactions

colesbury Jan 24, 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.

Maybe just _Py_TryIncref()? Also maybe rename src to ptr for consistency with the other functions here.

Sorry, something went wrong.

Uh oh!

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

DinoV reacted with thumbs up emoji

All reactions

👍 1 reaction

DinoV force-pushed the nogil_tryincref branch from 818fc5f to 3e50cbe Compare

January 24, 2024 20:40

colesbury approved these changes

Jan 24, 2024

View reviewed changes

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.

A few minor suggestions, but otherwise LGTM.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Include/internal/pycore_object.h Resolved

Uh oh!

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

Comment thread

Include/internal/pycore_object.h Resolved

Uh oh!

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

Comment thread

Include/internal/pycore_object.h Outdated


		/* Tries to incref the object op and ensures that src still points to it. /
		static inline int
		_Py_TryAcquireObject(PyObject *src, PyObject op)

colesbury Jan 24, 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.

Maybe just _Py_TryIncref()? Also maybe rename src to ptr for consistency with the other functions here.

Sorry, something went wrong.

Uh oh!

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

DinoV reacted with thumbs up emoji

All reactions

👍 1 reaction

DinoV force-pushed the nogil_tryincref branch from 3e50cbe to d5fe7a1 Compare

January 25, 2024 00:41

DinoV and others added 3 commits

January 24, 2024 16:45


          Bring in a subset of biased reference counting:

39c251e

colesbury/nogil@b6b12a9a94e

Co-Authored-By: Sam Gross <[email protected]>


          Review feedback

a923bc9


          More renames

87e9465

DinoV force-pushed the nogil_tryincref branch from d5fe7a1 to 87e9465 Compare

January 25, 2024 00:46

DinoV merged commit 4850410 into python:main

Jan 25, 2024

bedevere-app Bot removed the awaiting core review label

Jan 25, 2024

aisk pushed a commit to aisk/cpython that referenced this pull request

Feb 11, 2024


          pythongh-112075: Add try-incref functions from nogil branch for use i…

d6bc7b9

…n dict thread safety (python#114512)

* Bring in a subset of biased reference counting:
colesbury/nogil@b6b12a9a94e

The NoGIL branch has functions for attempting to do an incref on an object which may or may not be in flight. This just brings those functions over so that they will be usable from in the dict implementation to get items w/o holding a lock.

There's a handful of small simple modifications:

    Adding inline to the force inline functions to avoid a warning, and switching from _Py_ALWAYS_INLINE to Py_ALWAYS_INLINE as that's available
    Remove _Py_REF_LOCAL_SHIFT as it doesn't exist yet (and is currently 0 in the 3.12 nogil branch anyway)
    ob_ref_shared is currently Py_ssize_t and not uint32_t, so use that
    _PY_LIKELY doesn't exist, so drop it
    _Py_ThreadLocal becomes _Py_IsOwnedByCurrentThread
    Add '_PyInterpreterState_GET()' to _Py_IncRefTotal calls.


Co-Authored-By: Sam Gross <[email protected]>

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-112075: Add try-incref functions from nogil branch for use i…

f35e2a9

…n dict thread safety (python#114512)

* Bring in a subset of biased reference counting:
colesbury/nogil@b6b12a9a94e

The NoGIL branch has functions for attempting to do an incref on an object which may or may not be in flight. This just brings those functions over so that they will be usable from in the dict implementation to get items w/o holding a lock.

There's a handful of small simple modifications:

    Adding inline to the force inline functions to avoid a warning, and switching from _Py_ALWAYS_INLINE to Py_ALWAYS_INLINE as that's available
    Remove _Py_REF_LOCAL_SHIFT as it doesn't exist yet (and is currently 0 in the 3.12 nogil branch anyway)
    ob_ref_shared is currently Py_ssize_t and not uint32_t, so use that
    _PY_LIKELY doesn't exist, so drop it
    _Py_ThreadLocal becomes _Py_IsOwnedByCurrentThread
    Add '_PyInterpreterState_GET()' to _Py_IncRefTotal calls.


Co-Authored-By: Sam Gross <[email protected]>

Yhg1s mentioned this pull request

Jan 28, 2025

gh-115999: Add free-threaded specialization for FOR_ITER #128798

Merged

Reviewers

colesbury

colesbury approved these changes

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.

2 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

DinoV commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colesbury Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

DinoV Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

colesbury Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

colesbury Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DinoV commented Jan 24, 2024 •

edited

Loading

Uh oh!

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