From 2d5c312f8808a57808e3783706161edeefb725c4 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Fri, 1 Nov 2024 09:36:54 +0300 Subject: [PATCH 1/3] gh-103951: enable optimization for fast attribute access on module subclasses This patch relax specialization requirements from ``PyModule_CheckExact(op)`` to ``Py_TYPE(op)->tp_getattro != PyModule_Type.tp_getattro``. Benchmarks: ```py import pyperf import b import c runner = pyperf.Runner() runner.timeit('b.x', 'b.x', globals=globals()) runner.timeit('c.x', 'c.x', globals=globals()) ``` ```py x = 1 ``` ```py import sys, types x = 1 class _Foo(types.ModuleType): pass sys.modules[__name__].__class__ = _Foo ``` On the main: ``` $ python a.py -q b.x: Mean +- std dev: 50.2 ns +- 2.7 ns c.x: Mean +- std dev: 132 ns +- 7 ns ``` With the patch: ``` $ python a.py -q b.x: Mean +- std dev: 52.9 ns +- 3.6 ns c.x: Mean +- std dev: 52.6 ns +- 2.7 ns ``` Co-authored-by: Nicolas Tessore --- Include/internal/pycore_moduleobject.h | 6 ++++++ .../2024-11-01-09-58-06.gh-issue-103951.6qduwj.rst | 2 ++ Python/bytecodes.c | 2 +- Python/generated_cases.c.h | 2 +- Python/specialize.c | 2 +- Tools/cases_generator/analyzer.py | 1 + 6 files changed, 12 insertions(+), 3 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-11-01-09-58-06.gh-issue-103951.6qduwj.rst diff --git a/Include/internal/pycore_moduleobject.h b/Include/internal/pycore_moduleobject.h index 9bb282a13a9659..2682be6ad735ab 100644 --- a/Include/internal/pycore_moduleobject.h +++ b/Include/internal/pycore_moduleobject.h @@ -16,6 +16,12 @@ extern int _PyModule_IsPossiblyShadowing(PyObject *); extern int _PyModule_IsExtension(PyObject *obj); +static inline int +_PyModule_HasSpecializedGetAttr(PyObject* op) +{ + return Py_TYPE(op)->tp_getattro != PyModule_Type.tp_getattro; +} + typedef struct { PyObject_HEAD PyObject *md_dict; diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-11-01-09-58-06.gh-issue-103951.6qduwj.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-01-09-58-06.gh-issue-103951.6qduwj.rst new file mode 100644 index 00000000000000..39b54e0b72556e --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-01-09-58-06.gh-issue-103951.6qduwj.rst @@ -0,0 +1,2 @@ +Relax optimization requirements to allow fast attribute access to module +subclasses. diff --git a/Python/bytecodes.c b/Python/bytecodes.c index fa98af12c69aef..b7f60acedab80c 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2108,7 +2108,7 @@ dummy_func( op(_CHECK_ATTR_MODULE, (dict_version/2, owner -- owner)) { PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); - DEOPT_IF(!PyModule_CheckExact(owner_o)); + DEOPT_IF(_PyModule_HasSpecializedGetAttr(owner_o)); PyDictObject *dict = (PyDictObject *)((PyModuleObject *)owner_o)->md_dict; assert(dict != NULL); DEOPT_IF(dict->ma_keys->dk_version != dict_version); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 632cbc7790a4d8..dedca9cd761006 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5542,7 +5542,7 @@ owner = stack_pointer[-1]; uint32_t dict_version = read_u32(&this_instr[2].cache); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); - DEOPT_IF(!PyModule_CheckExact(owner_o), LOAD_ATTR); + DEOPT_IF(_PyModule_HasSpecializedGetAttr(owner_o), LOAD_ATTR); PyDictObject *dict = (PyDictObject *)((PyModuleObject *)owner_o)->md_dict; assert(dict != NULL); DEOPT_IF(dict->ma_keys->dk_version != dict_version, LOAD_ATTR); diff --git a/Python/specialize.c b/Python/specialize.c index ae47809305a300..883803f321a80f 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1143,7 +1143,7 @@ _Py_Specialize_LoadAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *nam SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OTHER); fail = true; } - else if (PyModule_CheckExact(owner)) { + else if (!_PyModule_HasSpecializedGetAttr(owner)) { fail = specialize_module_load_attr(owner, instr, name); } else if (PyType_Check(owner)) { diff --git a/Tools/cases_generator/analyzer.py b/Tools/cases_generator/analyzer.py index 66ead741b87a2b..471a3c60af176b 100644 --- a/Tools/cases_generator/analyzer.py +++ b/Tools/cases_generator/analyzer.py @@ -598,6 +598,7 @@ def has_error_without_pop(op: parser.InstDef) -> bool: "_PyLong_Multiply", "_PyLong_Subtract", "_PyManagedDictPointer_IsValues", + "_PyModule_HasSpecializedGetAttr", "_PyObject_GC_IS_TRACKED", "_PyObject_GC_MAY_BE_TRACKED", "_PyObject_GC_TRACK", From 907beb48fe3dec62466e2f8b97eb1df55f420806 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Fri, 1 Nov 2024 10:41:17 +0300 Subject: [PATCH 2/3] +1 --- Python/executor_cases.c.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index ff4a0a52a0b445..a3034013863a56 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2593,7 +2593,7 @@ owner = stack_pointer[-1]; uint32_t dict_version = (uint32_t)CURRENT_OPERAND(); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); - if (!PyModule_CheckExact(owner_o)) { + if (_PyModule_HasSpecializedGetAttr(owner_o)) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } From 3fafd937e7d57d469e4f66029f1ee57ac606ab74 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Thu, 7 Nov 2024 14:59:15 +0300 Subject: [PATCH 3/3] address review: inline checker code --- Include/internal/pycore_moduleobject.h | 6 ------ Python/bytecodes.c | 2 +- Python/executor_cases.c.h | 2 +- Python/generated_cases.c.h | 2 +- Python/specialize.c | 2 +- Tools/cases_generator/analyzer.py | 1 - 6 files changed, 4 insertions(+), 11 deletions(-) diff --git a/Include/internal/pycore_moduleobject.h b/Include/internal/pycore_moduleobject.h index 2682be6ad735ab..9bb282a13a9659 100644 --- a/Include/internal/pycore_moduleobject.h +++ b/Include/internal/pycore_moduleobject.h @@ -16,12 +16,6 @@ extern int _PyModule_IsPossiblyShadowing(PyObject *); extern int _PyModule_IsExtension(PyObject *obj); -static inline int -_PyModule_HasSpecializedGetAttr(PyObject* op) -{ - return Py_TYPE(op)->tp_getattro != PyModule_Type.tp_getattro; -} - typedef struct { PyObject_HEAD PyObject *md_dict; diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 54d937f4c9e582..6a94276aca7c1d 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2132,7 +2132,7 @@ dummy_func( op(_CHECK_ATTR_MODULE, (dict_version/2, owner -- owner)) { PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); - DEOPT_IF(_PyModule_HasSpecializedGetAttr(owner_o)); + DEOPT_IF(Py_TYPE(owner_o)->tp_getattro != PyModule_Type.tp_getattro); PyDictObject *dict = (PyDictObject *)((PyModuleObject *)owner_o)->md_dict; assert(dict != NULL); DEOPT_IF(dict->ma_keys->dk_version != dict_version); diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 83aafffd9674bd..def7fb1d3f64a8 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2602,7 +2602,7 @@ owner = stack_pointer[-1]; uint32_t dict_version = (uint32_t)CURRENT_OPERAND(); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); - if (_PyModule_HasSpecializedGetAttr(owner_o)) { + if (Py_TYPE(owner_o)->tp_getattro != PyModule_Type.tp_getattro) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index d52a1e7930c23d..eb2368538c2aec 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5561,7 +5561,7 @@ owner = stack_pointer[-1]; uint32_t dict_version = read_u32(&this_instr[2].cache); PyObject *owner_o = PyStackRef_AsPyObjectBorrow(owner); - DEOPT_IF(_PyModule_HasSpecializedGetAttr(owner_o), LOAD_ATTR); + DEOPT_IF(Py_TYPE(owner_o)->tp_getattro != PyModule_Type.tp_getattro, LOAD_ATTR); PyDictObject *dict = (PyDictObject *)((PyModuleObject *)owner_o)->md_dict; assert(dict != NULL); DEOPT_IF(dict->ma_keys->dk_version != dict_version, LOAD_ATTR); diff --git a/Python/specialize.c b/Python/specialize.c index 5ca6078ea0f63d..4c8cf8534b3dc7 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -1219,7 +1219,7 @@ _Py_Specialize_LoadAttr(_PyStackRef owner_st, _Py_CODEUNIT *instr, PyObject *nam SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OTHER); fail = true; } - else if (!_PyModule_HasSpecializedGetAttr(owner)) { + else if (Py_TYPE(owner)->tp_getattro == PyModule_Type.tp_getattro) { fail = specialize_module_load_attr(owner, instr, name); } else if (PyType_Check(owner)) { diff --git a/Tools/cases_generator/analyzer.py b/Tools/cases_generator/analyzer.py index 4e21ddcd8eeb56..a725ec10d4e52a 100644 --- a/Tools/cases_generator/analyzer.py +++ b/Tools/cases_generator/analyzer.py @@ -598,7 +598,6 @@ def has_error_without_pop(op: parser.InstDef) -> bool: "_PyLong_Multiply", "_PyLong_Subtract", "_PyManagedDictPointer_IsValues", - "_PyModule_HasSpecializedGetAttr", "_PyObject_GC_IS_TRACKED", "_PyObject_GC_MAY_BE_TRACKED", "_PyObject_GC_TRACK",