gh-110481: Implement biased reference counting #110764
Merged
Conversation
This file contains 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
|
Note for reviewer: the first commit in this PR refactors the internal-only |
|
@vstinner, will you have time to look at this PR this week? |
|
I read your PEP and related papers, so I can review for specific implementation of your works. I will leave review for this PR in this week :) |
corona10
reviewed
Oct 18, 2023
Include/internal/pycore_object.h
Outdated
| } | ||
| #define _Py_RefcntAdd(op, n) _Py_RefcntAdd(_PyObject_CAST(op), n) | ||
|
|
||
| static inline void _Py_SetImmortal(PyObject *op) | ||
| { | ||
| if (op) { | ||
| #ifdef Py_NOGIL | ||
| op->ob_tid = 0; |
There was a problem hiding this comment.
Can we declare the constant variable for this? Something like UNKNOWN_TID?
I mean if someone already read the PEP, the person knows what the state that tid is zero stands for, but if not some kind of variable name will be helpful to read.
But I am not sure which will be the best naming
There was a problem hiding this comment.
I've named it _Py_UNOWNED_TID because these are objects that are not owned by any thread.
corona10
reviewed
Oct 18, 2023
corona10
reviewed
Oct 18, 2023
Include/object.h
Outdated
| // zero. The call will deallocate the object if the shared refcount is also | ||
| // zero. Otherwise, the thread gives up ownership and merges the reference | ||
| // count fields. | ||
| PyAPI_FUNC(void) _Py_MergeZeroRefcount(PyObject *); |
There was a problem hiding this comment.
When I read the code, the purpose of this function is to handle the object when the local ref count is zero.
So might be, we can improve function name to understand what's happening
Suggested change
| PyAPI_FUNC(void) _Py_MergeZeroRefcount(PyObject *); | |
| PyAPI_FUNC(void) _Py_MergeZeroRefLocalcount(PyObject *); |
or
Suggested change
| PyAPI_FUNC(void) _Py_MergeZeroRefcount(PyObject *); | |
| PyAPI_FUNC(void) _Py_MergeZeroLocalRefcount(PyObject *); |
There was a problem hiding this comment.
I've named it _Py_MergeZeroLocalRefcount
corona10
approved these changes
Oct 18, 2023
corona10
reviewed
Oct 19, 2023
| Py_ssize_t | ||
| _Py_ExplicitMergeRefcount(PyObject *op, Py_ssize_t extra) | ||
| { | ||
| Py_ssize_t refcnt; |
There was a problem hiding this comment.
Out of curiosity, Don't we have to add assert statement that op should not be an immortal object?
assert(!_Py_IsImmortal(op));There was a problem hiding this comment.
Yeah, that's a good assertion to add.
colesbury
force-pushed
the
biased_reference_counting
branch
from
2144e0e
to
9bc2b6b
Compare
colesbury
requested review from
brettcannon,
ericsnowcurrently,
ncoghlan,
warsaw and
tiran
as code owners
colesbury
commented
Oct 19, 2023
Include/internal/pycore_object.h
Outdated
| } | ||
| #define _Py_RefcntAdd(op, n) _Py_RefcntAdd(_PyObject_CAST(op), n) | ||
|
|
||
| static inline void _Py_SetImmortal(PyObject *op) | ||
| { | ||
| if (op) { | ||
| #ifdef Py_NOGIL | ||
| op->ob_tid = 0; |
There was a problem hiding this comment.
I've named it _Py_UNOWNED_TID because these are objects that are not owned by any thread.
Include/object.h
Outdated
| // zero. The call will deallocate the object if the shared refcount is also | ||
| // zero. Otherwise, the thread gives up ownership and merges the reference | ||
| // count fields. | ||
| PyAPI_FUNC(void) _Py_MergeZeroRefcount(PyObject *); |
There was a problem hiding this comment.
I've named it _Py_MergeZeroLocalRefcount
| Py_ssize_t | ||
| _Py_ExplicitMergeRefcount(PyObject *op, Py_ssize_t extra) | ||
| { | ||
| Py_ssize_t refcnt; |
There was a problem hiding this comment.
Yeah, that's a good assertion to add.
corona10
reviewed
Oct 19, 2023
corona10
requested changes
Oct 20, 2023
There was a problem hiding this comment.
@colesbury In my macOS, the PR is failed to build :) Pleace check
9 warnings generated.
gcc -fno-strict-overflow -Wsign-compare -Wunreachable-code -DNDEBUG -g -O3 -Wall -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden -I./Include/internal -I. -I./Include -c ./Modules/xxsubtype.c -o Modules/xxsubtype.o
In file included from ./Modules/_scproxy.c:9:
In file included from ./Include/Python.h:52:
./Include/object.h:276:22: error: implicit declaration of function '_Py_atomic_load_uint32_relaxed' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
uint32_t local = _Py_atomic_load_uint32_relaxed(&ob->ob_ref_local);
^
./Include/object.h:280:25: error: implicit declaration of function '_Py_atomic_load_ssize_relaxed' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&ob->ob_ref_shared);
^
./Include/object.h:280:25: note: did you mean '_Py_atomic_load_uint32_relaxed'?
./Include/object.h:276:22: note: '_Py_atomic_load_uint32_relaxed' declared here
uint32_t local = _Py_atomic_load_uint32_relaxed(&ob->ob_ref_local);
^
./Include/object.h:343:9: error: implicit declaration of function '_Py_IsOnwedByCurrentThread' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
if (_Py_IsOnwedByCurrentThread(ob)) {
^
These aren't necessary for correctness, just performance.
corona10
force-pushed
the
biased_reference_counting
branch
from
d0b6720
to
ac2c957
Compare
corona10
approved these changes
Oct 30, 2023
There was a problem hiding this comment.
@colesbury cc @vstinner @ambv
LGTM Again
Let's not block the PEP-703 works for the things.
It will not affect the default implementation, and for the limited C API issue, I believe that we can solve it before we want to use free-threaded mode by default.
Let's solve it as a separate issue.
|
|
|
|
|
For the build bot issue, please follow up: #111503 |
|
For the limited C API, I wrote a first change: PR #111505. |
|
Congrats @colesbury, that's a massive change :-) |
dimaqq
reviewed
Oct 31, 2023
Comment on lines
+251
to
+257
| #elif defined(__arm__) | ||
| __asm__ ("mrc p15, 0, %0, c13, c0, 3\nbic %0, %0, #3" : "=r" (tid)); | ||
| #elif defined(__aarch64__) && defined(__APPLE__) | ||
| __asm__ ("mrs %0, tpidrro_el0" : "=r" (tid)); | ||
| #elif defined(__aarch64__) | ||
| __asm__ ("mrs %0, tpidr_el0" : "=r" (tid)); | ||
| #else |
There was a problem hiding this comment.
Caveat: I'm not an expert.
In case of x86/x86-64 above, the tid values has to be copied to a regular register for comparison.
In case of ARM32, the comparison could be made directly, were it not for the 2 lsb.
In case of ARM64, the comparison could be made directly.
Leaving aside the less common case of stamping the biased reference count block, and focusing on the more common case of comparing the current tid with object owner tid:
Does the compiler optimise this move away?
Does it perhaps not matter, as move is fast compared to loading the object's value from ram or cache?
Or would a custom comparison macro make sense here?
There was a problem hiding this comment.
This is backwards: on x86/x86-64 you can do a comparison without copying tid to a register; you can't on ARM.
On x86, many instructions, such as cmp, can operate on memory addresses, including addresses with fs: or gs: prefixes. It doesn't matter much here because because the object's owner tid also requires a memory load. So either one or the other needs to be moved to a temporary registers and it's more convenient to move the current tid to a temporary register because it may be re-used across multiple reference count operations.
On ARM you can't do a comparison with a system or coprocessor register. You need to move it to a gpr first.
|
I ran this PR on the Faster CPython team's benchmarking infrastructure, and its effect is basically immeasurable, 1.00x slower with a reliability of 95%: https://github.com/faster-cpython/benchmarking-public/blob/main/results/bm-20231030-3.13.0a1%2B-6dfb8fe/bm-20231030-linux-x86_64-python-6dfb8fe0236718e9afc8-3.13.0a1%2B-6dfb8fe-vs-base.md EDIT: As @ericsnowcurrently pointed out, this is with a default build, so is not terribly interesting. I'll rerun with |
This PR only impacts Python if Python is configured with |
|
And the correct benchmarking results are in. With |
|
@colesbury: An interesting thing the benchmarking revealed is the |
|
@mdboom, Yeah the
|
FullteaR
pushed a commit
to FullteaR/cpython
that referenced
this pull request
Nov 3, 2023
aisk
pushed a commit
to aisk/cpython
that referenced
this pull request
Feb 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
CPython's current reference counting implementation would not be thread-safe without the GIL. This implements biased reference counting in
--disable-gilbuilds, which is thread-safe and has lower execution overhead compared to plain atomic reference counting. The design is described in the "Biased Reference Counting" section of PEP 703.--disable-gilbuilds #110481