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-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build by corona10 · Pull Request #111560 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Oct 31, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I simply implement _Py_DECREF_NO_DEALLOC for the free-threaded build.
I assumed refcount will not be zero even after calling _Py_DECREF_NO_DEALLOC.

Please let me know if I missed something.

  • Issue: Implement biased reference counting in --disable-gil builds #110481

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
pythongh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build
6c2ffb8
@corona10 corona10 added the topic-free-threading label Oct 31, 2023
@corona10
corona10 requested a review from colesbury October 31, 2023 16:43
@bedevere-app bedevere-app Bot mentioned this pull request Oct 31, 2023
Implement biased reference counting in --disable-gil builds #110481
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Oct 31, 2023
@corona10 corona10 added the skip news label Oct 31, 2023
corona10 added 2 commits November 1, 2023 01:47
@corona10
Remove assert
397f018
@corona10
Update
2c2f4df
@corona10 corona10 changed the title gh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build [WIP] gh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build Oct 31, 2023
@corona10
Update
8166ae0
@corona10 corona10 added the DO-NOT-MERGE label Oct 31, 2023
@corona10 corona10 changed the title [WIP] gh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build gh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build Oct 31, 2023
@corona10 corona10 removed the DO-NOT-MERGE label Oct 31, 2023
corona10
corona10 commented Oct 31, 2023
View reviewed changes
Comment thread Include/internal/pycore_object.h
Py_ssize_t refcount = _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared);
Py_ssize_t new_shared;
// Shared refcount can be zero but we should consider local refcount.
int should_queue = (refcount == 0 || refcount == _Py_REF_MAYBE_WEAKREF);

@corona10 corona10 Oct 31, 2023

Copy link
Copy Markdown
Member 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.

@colesbury Out of curiosity, Don't we have to consider that shared_refcount can be a negative value at this moment due to imbalance refcounting?

Same question for the

cpython/Objects/object.c

Line 319 in 2445673

should_queue = (shared == 0 || shared == _Py_REF_MAYBE_WEAKREF);

Sorry, something went wrong.

Uh oh!

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

@colesbury colesbury Oct 31, 2023

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'm not sure I fully understand your question. Yes, shared_refcount may be negative. should_queue is always false if shared is negative because:

  1. we only queue objects once
  2. we queue them the first time the shared refcount becomes negative

So if it's already negative then we must have already queued it.

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

@corona10 corona10 Oct 31, 2023

Copy link
Copy Markdown
Member 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.

So if it's already negative then we must have already queued it.

Make sense, thank you for explain.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
corona10
corona10 commented Oct 31, 2023
View reviewed changes
Comment thread Include/internal/pycore_object.h
assert(refcount != 0);
refcount--;
_Py_atomic_store_uint32_relaxed(&op->ob_ref_local, refcount);
if (refcount == 0) {

@corona10 corona10 Oct 31, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member 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.

Similar question, do we have to handle zero local refcounting cases from _Py_DECREF_NO_DEALLOC
or it can be handled as deferred merging from somewhere?

Sorry, something went wrong.

Uh oh!

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

@colesbury colesbury Oct 31, 2023

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.

Yes, you need _Py_MergeZeroLocalRefcount. You can't defer it -- that would break a bunch of invariants. For example, the same thread may try calling Py_DECREF again leading to a negative local refcount.

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
@corona10 corona10 closed this Oct 31, 2023
colesbury
colesbury reviewed Oct 31, 2023
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.

I'm not sure this change will actually improve performance. In the default build, _Py_DECREF_NO_DEALLOC can avoid a comparison and avoids emitting a function call. With Py_NOGIL, we still need the comparison and call to _Py_MergeZeroLocalRefcount.

In the end, I think the Py_NOGIL version of _Py_DECREF_NO_DEALLOC may look exactly like Py_DECREF.

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_object.h
assert(refcount != 0);
refcount--;
_Py_atomic_store_uint32_relaxed(&op->ob_ref_local, refcount);
if (refcount == 0) {

@colesbury colesbury Oct 31, 2023

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.

Yes, you need _Py_MergeZeroLocalRefcount. You can't defer it -- that would break a bunch of invariants. For example, the same thread may try calling Py_DECREF again leading to a negative local refcount.

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_object.h
Py_ssize_t refcount = _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared);
Py_ssize_t new_shared;
// Shared refcount can be zero but we should consider local refcount.
int should_queue = (refcount == 0 || refcount == _Py_REF_MAYBE_WEAKREF);

@colesbury colesbury Oct 31, 2023

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'm not sure I fully understand your question. Yes, shared_refcount may be negative. should_queue is always false if shared is negative because:

  1. we only queue objects once
  2. we queue them the first time the shared refcount becomes negative

So if it's already negative then we must have already queued it.

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
@corona10 corona10 reopened this Oct 31, 2023
@corona10

corona10 commented Oct 31, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I'm not sure this change will actually improve performance. In the default build, _Py_DECREF_NO_DEALLOC can avoid a > comparison and avoids emitting a function call. With Py_NOGIL, we still need the comparison and call to > _Py_MergeZeroLocalRefcount.

In the end, I think the Py_NOGIL version of _Py_DECREF_NO_DEALLOC may look exactly like Py_DECREF.

I totally agree. If we consider edge cases of _Py_DECREF_NO_DEALLOC, it will be similar to Py_DECREF of Py_NOGIL version.

I close the PR, and if there is a way to improve the performance, I will re-investigate it.
Thank you!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 closed this Oct 31, 2023
@corona10
corona10 deleted the _Py_DECREF_NO_DEALLOC branch November 1, 2023 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury left review comments

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review 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.