Skip to content

Commit b5567ae

Browse files
alexmalyshevmeta-codesync[bot]
authored andcommitted
Backport gh-132657 to 3.14: defer refcount on module values
Summary: Backport of upstream gh-132657, "If we are specializing to LOAD_GLOBAL_MODULE or LOAD_ATTR_MODULE, try to enable deferred reference counting for the value, if the object is owned by a different thread", which landed in CPython 3.15.0a6. Meta Python 3.14 has neither the helper nor the calls; `third-party/python/main` already has it via the upstream import. Stacked on the gh-143469 backport because they touch the same function and only make sense together: gh-143469 lets the module attribute load specialize at all, and this one makes the value it caches cheap to load from many threads. Either alone leaves the common case flat. `maybe_enable_deferred_ref_count` is copied verbatim from upstream. The two call sites needed adapting: upstream reads the value out of `_PyDict_LookupIndexAndValue`, which does not exist in 3.14, so the value comes from the dict entry instead. That can be NULL for a deleted key at a still-valid index, which upstream's helper input cannot be, so the call is NULL-guarded at both sites. Both opcodes already deopt on a NULL entry value at run time, so behaviour is unchanged. Scope note: upstream also calls this helper from the class-attribute descriptor path. That is a different change, and 3.14's version of that code currently *fails* specialization when the descriptor lacks deferred refcount (`SPEC_FAIL_ATTR_DESCR_NOT_DEFERRED`) rather than enabling it, so converting it is not a mechanical backport. Left alone; gh-132657's own NEWS entry names only `LOAD_GLOBAL_MODULE` and `LOAD_ATTR_MODULE`. Measured on 3.14.7t against this stack, three modules differing only in the value they expose and whether they define `__getattr__`, speedup relative to one thread: module value 1t ms 8t 32t mortal, no __getattr__ 10.9 9.0x 19.2x mortal, has __getattr__ 9.1 7.8x 16.8x immortal, has __getattr__ 9.2 5.0x 16.6x untracked, has __getattr__ 11.0 0.5x 0.5x The second row is the one this diff buys: with only the gh-143469 backport below it, a mortal module value scored 0.5x. It is now 16.8x, level with an immortal one. The last row is the limit, and it is deliberate upstream behaviour rather than a gap in the backport: the helper is gated on `_PyObject_GC_IS_TRACKED`, and a plain `object()` is not tracked, so it is skipped. That is exactly the shape of torch's `dtype` / `layout` / `memory_format` / `qscheme` singletons, which is why the torch diff above this one still has to make those types GC-tracked before anything can help them. CinderX has to move with this. `UpstreamBorrow` copies `specialize_module_load_attr_lock_held` and `specialize_load_global_lock_held` out of `Python/specialize.c` and recompiles them, and borrowing is per function rather than transitive, so the new static helper they now call was not carried across and the free-threaded cinderx build failed with an implicit-declaration error. Adding a `Borrow` directive for it to `borrowed-3.14.free-threading.c.template` fixes that. Only the free-threaded template needs it; the helper and both call sites are inside `#ifdef Py_GIL_DISABLED`, so the GIL build compiles them out. `borrowed-3.14.free-threading.gen_cached.c` is regenerated to match. That file is a checked-in snapshot and is not consumed by this build config (`borrowed_library` is called with the default `cached = False`, so the genrule regenerates from the template), but it is generated output and should not be left stale. Its delta folds in the body change from the gh-143469 backport below as well, since that commit alters the same borrowed function and nothing regenerated the snapshot at that point; no build breaks in between, because nothing compiles the snapshot. Validated end to end. Adding `bundle_runtime = True` to a `python_binary` temporarily builds it against the interpreter from this commit, so the whole stack can be exercised at once. Probe results at 32 threads, stock 3.14t against this stack: case stock stack touch np.zeros 0.1x 24.7x touch torch.empty 0.0x 18.3x touch torch.long 0.1x 16.8x touch torch.float32 0.1x 16.6x touch torch.Tensor 0.1x 16.7x torch.empty(4) 0.1x 11.7x as_tensor(dtype=long) 0.3x 2.1x numpy is fixed for free by the two CPython backports, with no numpy change. Two rows are unmoved and both are expected: `touch math.pi` (0.8x) and `touch shared obj` (0.5x) are mortal objects that are not GC-tracked, which `maybe_enable_deferred_ref_count` skips by design. `as_tensor(dtype=long)` reaching only 2.1x is the honest remaining gap. That row is an actual tensor *construction*, not a lookup, so what is left is inside torch: argument parsing, allocation, `TensorImpl` setup. Refcount contention on the namespace was never going to explain all of it, and this stack does not address it. Reviewed By: itamaro Differential Revision: D116032672 fbshipit-source-id: 2109b9ad7c9122e4635e04fe8f4bc2260ac11447
1 parent 14b5442 commit b5567ae

3 files changed

Lines changed: 48 additions & 0 deletions

File tree

cinderx/UpstreamBorrow/borrowed-3.14.free-threading.c.template

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -496,6 +496,7 @@ void _PyErr_SetObject(PyThreadState* tstate, PyObject* type, PyObject* value) {
496496
// @Borrow function load_counter from Python/specialize.c [3.14]
497497
// @Borrow function unspecialize from Python/specialize.c [3.14]
498498
// @Borrow function _Py_Specialize_LoadSuperAttr from Python/specialize.c [3.14]
499+
// @Borrow function maybe_enable_deferred_ref_count from Python/specialize.c [3.14]
499500
// @Borrow function specialize_module_load_attr_lock_held from Python/specialize.c [3.14]
500501
// @Borrow function specialize_module_load_attr from Python/specialize.c [3.14]
501502
// @Borrow function classify_descriptor from Python/specialize.c [3.14]

cinderx/UpstreamBorrow/borrowed-3.14.free-threading.gen_cached.c

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5016,6 +5016,17 @@ _Py_Specialize_LoadSuperAttr(_PyStackRef global_super_st, _PyStackRef cls_st, _P
50165016
fail:
50175017
unspecialize(instr);
50185018
}
5019+
static void
5020+
maybe_enable_deferred_ref_count(PyObject *op)
5021+
{
5022+
if (!_Py_IsOwnedByCurrentThread(op) && _PyObject_GC_IS_TRACKED(op)) {
5023+
// For module level variables that are heavily used from multiple
5024+
// threads, deferred reference counting provides good scaling
5025+
// benefits. The downside is that the object will only be deallocated
5026+
// by a GC run.
5027+
PyUnstable_Object_EnableDeferredRefcount(op);
5028+
}
5029+
}
50195030
static int
50205031
specialize_module_load_attr_lock_held(PyDictObject *dict, _Py_CODEUNIT *instr, PyObject *name)
50215032
{
@@ -5054,6 +5065,16 @@ specialize_module_load_attr_lock_held(PyDictObject *dict, _Py_CODEUNIT *instr, P
50545065
SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS);
50555066
return -1;
50565067
}
5068+
#ifdef Py_GIL_DISABLED
5069+
// Upstream reads the value out of `_PyDict_LookupIndexAndValue`, which
5070+
// 3.14 does not have, so take it from the entry instead. That can be NULL
5071+
// for a deleted key at a still-valid index, which `_LOAD_ATTR_MODULE`
5072+
// handles at run time via `DEOPT_IF(attr_o == NULL)`.
5073+
PyObject *value = DK_UNICODE_ENTRIES(dict->ma_keys)[index].me_value;
5074+
if (value != NULL) {
5075+
maybe_enable_deferred_ref_count(value);
5076+
}
5077+
#endif
50575078
write_u32(cache->version, keys_version);
50585079
cache->index = (uint16_t)index;
50595080
specialize(instr, LOAD_ATTR_MODULE);
@@ -5904,6 +5925,14 @@ specialize_load_global_lock_held(
59045925
SPECIALIZATION_FAIL(LOAD_GLOBAL, SPEC_FAIL_OUT_OF_RANGE);
59055926
goto fail;
59065927
}
5928+
#ifdef Py_GIL_DISABLED
5929+
// Same as above: no `_PyDict_LookupIndexAndValue` in 3.14, and
5930+
// `_LOAD_GLOBAL_MODULE` deopts on a NULL entry value itself.
5931+
PyObject *value = DK_UNICODE_ENTRIES(globals_keys)[index].me_value;
5932+
if (value != NULL) {
5933+
maybe_enable_deferred_ref_count(value);
5934+
}
5935+
#endif
59075936
cache->index = (uint16_t)index;
59085937
cache->module_keys_version = (uint16_t)keys_version;
59095938
specialize(instr, LOAD_GLOBAL_MODULE);

cinderx/UpstreamBorrow/borrowed-3.14.gen_cached.c

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5170,6 +5170,16 @@ specialize_module_load_attr_lock_held(PyDictObject *dict, _Py_CODEUNIT *instr, P
51705170
SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OUT_OF_VERSIONS);
51715171
return -1;
51725172
}
5173+
#ifdef Py_GIL_DISABLED
5174+
// Upstream reads the value out of `_PyDict_LookupIndexAndValue`, which
5175+
// 3.14 does not have, so take it from the entry instead. That can be NULL
5176+
// for a deleted key at a still-valid index, which `_LOAD_ATTR_MODULE`
5177+
// handles at run time via `DEOPT_IF(attr_o == NULL)`.
5178+
PyObject *value = DK_UNICODE_ENTRIES(dict->ma_keys)[index].me_value;
5179+
if (value != NULL) {
5180+
maybe_enable_deferred_ref_count(value);
5181+
}
5182+
#endif
51735183
write_u32(cache->version, keys_version);
51745184
cache->index = (uint16_t)index;
51755185
specialize(instr, LOAD_ATTR_MODULE);
@@ -6020,6 +6030,14 @@ specialize_load_global_lock_held(
60206030
SPECIALIZATION_FAIL(LOAD_GLOBAL, SPEC_FAIL_OUT_OF_RANGE);
60216031
goto fail;
60226032
}
6033+
#ifdef Py_GIL_DISABLED
6034+
// Same as above: no `_PyDict_LookupIndexAndValue` in 3.14, and
6035+
// `_LOAD_GLOBAL_MODULE` deopts on a NULL entry value itself.
6036+
PyObject *value = DK_UNICODE_ENTRIES(globals_keys)[index].me_value;
6037+
if (value != NULL) {
6038+
maybe_enable_deferred_ref_count(value);
6039+
}
6040+
#endif
60236041
cache->index = (uint16_t)index;
60246042
cache->module_keys_version = (uint16_t)keys_version;
60256043
specialize(instr, LOAD_GLOBAL_MODULE);

0 commit comments

Comments
 (0)