FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build by corona10 · Pull Request #111560 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build - #111560

Closed
corona10 wants to merge 4 commits into
python:mainfrom
corona10:_Py_DECREF_NO_DEALLOC
ConversationCommits (4)ChecksFiles changed
Closed

gh-110481: Implement _Py_DECREF_NO_DEALLOC for free-threaded build#111560
corona10 wants to merge 4 commits into
python:mainfrom
corona10:_Py_DECREF_NO_DEALLOC

Conversation

corona10 commented Oct 31, 2023 •
edited
Loading

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.

corona10 requested a review from colesbury October 31, 2023 16:43
bedevere-app Bot mentioned this pull request Oct 31, 2023
Closed
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 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 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 Oct 31, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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);

colesbury Oct 31, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

corona10 Oct 31, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Make sense, thank you for explain.

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 Oct 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

colesbury Oct 31, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

corona10 closed this Oct 31, 2023
colesbury reviewed Oct 31, 2023
View reviewed changes

colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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 Oct 31, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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 Oct 31, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

corona10 reopened this Oct 31, 2023

corona10 commented Oct 31, 2023 •
edited
Loading

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!

corona10 closed this Oct 31, 2023
corona10 deleted the _Py_DECREF_NO_DEALLOC branch November 1, 2023 00:32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

colesbury colesbury left review comments

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.

2 participants

Back | FazBrowse Home | New Git URL