Skip to content

gh-112075: Add try-incref functions from nogil branch for use in dict thread safety#114512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
DinoV merged 3 commits into from
Jan 25, 2024
Merged

gh-112075: Add try-incref functions from nogil branch for use in dict thread safety #114512

DinoV merged 3 commits into from
Jan 25, 2024

Conversation

@DinoV
Copy link
Contributor

@DinoVDinoV commented Jan 24, 2024•
edited
Loading

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:

  1. 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
  2. Remove _Py_REF_LOCAL_SHIFT as it doesn't exist yet (and is currently 0 in the 3.12 nogil branch anyway)
  3. ob_ref_shared is currently Py_ssize_t and not uint32_t, so use that
  4. _PY_LIKELY doesn't exist, so drop it
  5. _Py_ThreadLocal becomes _Py_IsOwnedByCurrentThread
  6. Add '_PyInterpreterState_GET()' to _Py_IncRefTotal calls.

@bedevere-appbedevere-appbot mentioned this pull request Jan 24, 2024
Closed
@DinoVDinoVforce-pushed the nogil_tryincref branch 4 times, most recently from 9318df9 to 818fc5fCompareJanuary 24, 2024 18:52
@DinoVDinoV requested a review from colesburyJanuary 24, 2024 19:10
@DinoVDinoV marked this pull request as ready for review January 24, 2024 19:25
colesbury
colesbury reviewed Jan 24, 2024
View reviewed changes
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)
Copy link
Contributor

@colesburycolesburyJan 24, 2024

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.

Copy link
ContributorAuthor

@DinoVDinoVJan 24, 2024

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

Copy link
Contributor

@colesburycolesburyJan 24, 2024

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.

colesbury
colesbury approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@colesburycolesbury left a 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.

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)
Copy link
Contributor

@colesburycolesburyJan 24, 2024

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.

@DinoVDinoV merged commit 4850410 into python:mainJan 25, 2024
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@DinoV@colesbury@aisk
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 <colesbury@gmail.com>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@DinoV@colesbury@Glyphack
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 <colesbury@gmail.com>
@Yhg1sYhg1s mentioned this pull request Jan 28, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesburycolesburycolesbury approved these changes

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.

2 participants

@DinoV@colesbury