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-111926: Set up basic sementics of weakref API for freethreading by corona10 · Pull Request #113621 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Jan 1, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: Make weakref thread-safe without the GIL #111926

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
pythongh-111926: Set up basic sementics of weakref API for freethreading
a400a06
@corona10 corona10 added skip news topic-free-threading labels Jan 1, 2024
@corona10
corona10 requested a review from colesbury January 1, 2024 15:54
@bedevere-app bedevere-app Bot added the awaiting core review label Jan 1, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jan 1, 2024
Make weakref thread-safe without the GIL #111926
Closed
@corona10

corona10 commented Jan 1, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@colesbury
_PyWeakref_GET_REF is same role of PyWeakref_FetchObject(at least for weakrefobject.c) which was originally proposed at PEP 703.
Since the PyWeakrefControl does not exist and is not needed (maybe) at the CPython now, critical section API is used for this.

Once API semantics are finalized for weakref-related APIs than we can make weakref object to be thread-safe at the free-threading building.

Please let me know if I miss something :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
fix compiler warn
5c755a8
@corona10 corona10 added the DO-NOT-MERGE label Jan 1, 2024
@corona10 corona10 changed the title gh-111926: Set up basic sementics of weakref API for freethreading [WIP] gh-111926: Set up basic sementics of weakref API for freethreading Jan 1, 2024
@corona10

corona10 commented Jan 1, 2024

Copy link
Copy Markdown
Member Author

Okay CI is failed, I need to check :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
fix
32b612f
@corona10 corona10 changed the title [WIP] gh-111926: Set up basic sementics of weakref API for freethreading gh-111926: Set up basic sementics of weakref API for freethreading Jan 1, 2024
@corona10 corona10 removed the DO-NOT-MERGE label Jan 1, 2024
colesbury
colesbury reviewed Jan 2, 2024
View reviewed changes
Comment thread Include/internal/pycore_weakref.h Outdated
Comment on lines +26 to +33
Py_ssize_t refcount = Py_REFCNT(obj);
if (refcount == 0) {
// In freethreading CPython, Py_REFCNT is only estimated count,
// so need to check shared refcount value too.
Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&obj->ob_ref_shared);
return _Py_REF_IS_MERGED(shared);
}
return 0;

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

This is a bit tricky. It's not enough to check that shared is merged -- the shared reference count must also be zero. The Py_REFCNT(obj) == 0 check may seem to imply this, but it doesn't both because it happens earlier in the code (and the reference count may change between the calls) and because the Py_REFCNT(obj) check itself is prone to race conditions since it needs to load the local and shared reference counts separately. That's why it's only an estimate.

It should be enough to only check the that the shared reference count is zero and that it is merged.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Include/internal/pycore_weakref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_weakref.h Outdated
return NULL;
static inline int _is_dead(PyObject *obj)
{
if (obj == NULL) {

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

Is this check necessary? It doesn't look like obj should every be NULL because wr_object uses Py_None for dead objects instead of NULL.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
colesbury
colesbury reviewed Jan 2, 2024
View reviewed changes
Comment thread Include/internal/pycore_weakref.h
ret = 1;
}
else {
// See _PyWeakref_GET_REF() for the rationale of this test

@colesbury colesbury Jan 2, 2024 •
edited
Loading

Uh oh!

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

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.

EDIT: removed suggestion

Sorry, something went wrong.

Uh oh!

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

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

Never mind -- that's probably not helpful. Can probably just delete the comment now that the code calls _is_dead(). The reader probably knows to look at the function implementation to understand the call.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10

corona10 commented Jan 2, 2024

Copy link
Copy Markdown
Member Author

Thanks for the review, I will take a look at tomorrow
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10 and others added 3 commits January 3, 2024 21:59
@corona10 @colesbury
Update Include/internal/pycore_weakref.h
1662b5d 
Co-authored-by: Sam Gross <[email protected]>
@corona10
Address code review
8b75804
@corona10
Sort header
72baa0f
@corona10
corona10 requested a review from colesbury January 3, 2024 13:02
colesbury
colesbury approved these changes Jan 3, 2024
View reviewed changes
@corona10
corona10 enabled auto-merge (squash) January 3, 2024 13:18
@corona10
corona10 merged commit 0c3455a into python:main Jan 3, 2024
@bedevere-app bedevere-app Bot removed the awaiting core review label Jan 3, 2024
@corona10
corona10 deleted the gh-111926-weakref-obj branch January 3, 2024 13:29
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
@corona10 @colesbury @kulikjak
pythongh-111926: Set up basic sementics of weakref API for freethread…
b7204b4 
…ing (pythongh-113621)

---------

Co-authored-by: Sam Gross <[email protected]>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@corona10 @colesbury @aisk
pythongh-111926: Set up basic sementics of weakref API for freethread…
994785b 
…ing (pythongh-113621)

---------

Co-authored-by: Sam Gross <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@corona10 @colesbury @Glyphack
pythongh-111926: Set up basic sementics of weakref API for freethread…
7d4b548 
…ing (pythongh-113621)

---------

Co-authored-by: Sam Gross <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury 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

@corona10 @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.