Skip to content

Commit 51a4794

Browse files
alexmalyshevmeta-codesync[bot]
authored andcommitted
Backport gh-143469 to 3.14: LOAD_ATTR_MODULE past __getattr__
Summary: This is a backport of upstream gh-143469, "Enable LOAD_ATTR_MODULE specialization even if __getattr__ is defined in module", which landed in CPython 3.15.0a5. Meta Python 3.14 is behind it: the check is still present in `third-party/python/3.14/pristine`, while `third-party/python/main` (3.16.0a0) already has it via the normal upstream import, so nothing is needed there. Claude arrived at the change independently before finding the upstream one; the reasoning below was generated first, then matched against what upstream did. Carrying it as a Meta patch until 3.14 syncs past the commit. `specialize_module_load_attr_lock_held` refuses to specialize any attribute load on a module whose dict contains `__getattr__`, whatever name is being looked up. That is stricter than it needs to be, and on a free-threaded build it is very expensive. A module `__getattr__` is only consulted when the name is *absent* from the module dict, and `_LOAD_ATTR_MODULE` already deoptimizes in exactly that case. A name absent at specialization time fails the existing `(uint16_t)` index check, since `DKIX_EMPTY` is `-1`. A name deleted afterwards leaves `ep->me_value` NULL, which the opcode's own `DEOPT_IF(attr_o == NULL)` catches, and control falls back to `module_getattro`, which runs `__getattr__` as before. Meta Python's lazy-import guard, `DEOPT_IF(PyLazyImport_CheckExact(attr_o))`, sits on the same path and is likewise unaffected. So the `__getattr__` probe is redundant with the opcode's guards and can go. Why it is worth doing: both `torch` and `numpy` define a module `__getattr__`, torch for lazy submodules (`_dynamo`, `_inductor`, `_export`, `onnx`) and deprecated attrs, numpy for its deprecated aliases. That single key means every `torch.foo` and every `np.foo` in the process takes the generic `PyObject_GetAttr` path forever, never reaching the lock-free `_Py_TryIncrefCompareStackRef` fast path in `_LOAD_ATTR_MODULE`. Measured with two identical modules differing only in whether they define `__getattr__`, both exposing the same immortal value so that reference counting is held constant. 3.14.7t, speedup relative to one thread: before, fbcode toolchain 3.14t: module 1t ms 8t 32t no __getattr__ 9.7 7.7x 15.5x has __getattr__ 17.2 0.5x 0.5x after, this change: no __getattr__ 9.1 7.6x 15.9x has __getattr__ 9.5 7.9x 17.6x 0.5x to 17.6x, and defining `__getattr__` now costs nothing. Note the 1t column too: 17.2ms to 9.5ms, so this is also ~1.8x on a single thread and is a plain win under the GIL, where those lookups were never specialized either. This is one of two independent causes, and neither fixes the real case alone. Torch's namespace callables and `dtype` singletons are also mortal, so even with this change a `torch.long` load still contends on the object's own refcount; the sibling `ft-torch-deferred-refcount` handles that. Direct evidence: on an interpreter with this change, the same control run with a *mortal* module value scores 0.5x, unchanged. 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: D116032673 fbshipit-source-id: 82bf047efdd0ccf41cef42b3b5a008af6c867c18
1 parent 34f1319 commit 51a4794

2 files changed

Lines changed: 32 additions & 14 deletions

File tree

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

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5024,13 +5024,22 @@ specialize_module_load_attr_lock_held(PyDictObject *dict, _Py_CODEUNIT *instr, P
50245024
SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_NON_STRING);
50255025
return -1;
50265026
}
5027-
Py_ssize_t index = _PyDict_LookupIndex(dict, &_Py_ID(__getattr__));
5028-
assert(index != DKIX_ERROR);
5029-
if (index != DKIX_EMPTY) {
5030-
SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_MODULE_ATTR_NOT_FOUND);
5031-
return -1;
5032-
}
5033-
index = _PyDict_LookupIndex(dict, name);
5027+
/* A module-level __getattr__ is consulted only when the name is absent
5028+
from the module dict, and _LOAD_ATTR_MODULE already deoptimizes in
5029+
exactly that case: a name absent now fails the uint16 index check
5030+
below, and one deleted later leaves ep->me_value NULL, which the
5031+
opcode's DEOPT_IF catches. So defining __getattr__ need not disable
5032+
specialization for the names the module does define.
5033+
5034+
This matters well beyond the rare fallback it was guarding. Both
5035+
`torch` and `numpy` define a module __getattr__ (for lazy submodules
5036+
and deprecated aliases), so every attribute access on them took the
5037+
generic path process-wide. On a free-threaded build that is the
5038+
difference between the lock-free _Py_TryIncrefCompareStackRef in
5039+
_LOAD_ATTR_MODULE and a contended refcount: measured over 32 threads,
5040+
`np.zeros` in a loop scaled 0.1x through the module against 16x for
5041+
the same object bound to a plain global. */
5042+
Py_ssize_t index = _PyDict_LookupIndex(dict, name);
50345043
assert (index != DKIX_ERROR);
50355044
if (index != (uint16_t)index) {
50365045
SPECIALIZATION_FAIL(LOAD_ATTR,

cinderx/UpstreamBorrow/borrowed-3.14.gen_cached.c

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5140,13 +5140,22 @@ specialize_module_load_attr_lock_held(PyDictObject *dict, _Py_CODEUNIT *instr, P
51405140
SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_NON_STRING);
51415141
return -1;
51425142
}
5143-
Py_ssize_t index = _PyDict_LookupIndex(dict, &_Py_ID(__getattr__));
5144-
assert(index != DKIX_ERROR);
5145-
if (index != DKIX_EMPTY) {
5146-
SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_MODULE_ATTR_NOT_FOUND);
5147-
return -1;
5148-
}
5149-
index = _PyDict_LookupIndex(dict, name);
5143+
/* A module-level __getattr__ is consulted only when the name is absent
5144+
from the module dict, and _LOAD_ATTR_MODULE already deoptimizes in
5145+
exactly that case: a name absent now fails the uint16 index check
5146+
below, and one deleted later leaves ep->me_value NULL, which the
5147+
opcode's DEOPT_IF catches. So defining __getattr__ need not disable
5148+
specialization for the names the module does define.
5149+
5150+
This matters well beyond the rare fallback it was guarding. Both
5151+
`torch` and `numpy` define a module __getattr__ (for lazy submodules
5152+
and deprecated aliases), so every attribute access on them took the
5153+
generic path process-wide. On a free-threaded build that is the
5154+
difference between the lock-free _Py_TryIncrefCompareStackRef in
5155+
_LOAD_ATTR_MODULE and a contended refcount: measured over 32 threads,
5156+
`np.zeros` in a loop scaled 0.1x through the module against 16x for
5157+
the same object bound to a plain global. */
5158+
Py_ssize_t index = _PyDict_LookupIndex(dict, name);
51505159
assert (index != DKIX_ERROR);
51515160
if (index != (uint16_t)index) {
51525161
SPECIALIZATION_FAIL(LOAD_ATTR,

0 commit comments

Comments
 (0)