Fast attribute access for module subclasses

[ntessore]

Feature or enhancement

There is a specialisation that allows for fast attribute access on the module type, and only the module type, since it is guarded by PyModule_CheckExact(). This specialisation could be enabled for module subclasses.

Pitch

There are occasionally reasons to subclass ModuleType to create module types which are more "class-like", e.g. to equip them with magic methods. These subclasses suffer from a performance penalty on attribute access because the specialisation is guarded against subclasses. I do not see the reason for this; in fact, the specialisation can be applied to all module subclasses with no ill effects (that I can observe) using a tiny patch:

Patch

diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index 9de0d92..ab948f5 100644
--- a/Python/bytecodes.c
+++ b/Python/bytecodes.c
@@ -1676,7 +1676,7 @@ dummy_func(
         }
 
         inst(LOAD_ATTR_MODULE, (unused/1, type_version/2, index/1, unused/5, owner -- res2 if (oparg & 1), res)) {
-            DEOPT_IF(!PyModule_CheckExact(owner), LOAD_ATTR);
+            DEOPT_IF(!PyModule_Check(owner), LOAD_ATTR);
             PyDictObject *dict = (PyDictObject *)((PyModuleObject *)owner)->md_dict;
             assert(dict != NULL);
             DEOPT_IF(dict->ma_keys->dk_version != type_version, LOAD_ATTR);
diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h
index 864a4f7..96c764f 100644
--- a/Python/generated_cases.c.h
+++ b/Python/generated_cases.c.h
@@ -2338,7 +2338,7 @@
             uint32_t type_version = read_u32(&next_instr[1].cache);
             uint16_t index = read_u16(&next_instr[3].cache);
             #line 1679 "Python/bytecodes.c"
-            DEOPT_IF(!PyModule_CheckExact(owner), LOAD_ATTR);
+            DEOPT_IF(!PyModule_Check(owner), LOAD_ATTR);
             PyDictObject *dict = (PyDictObject *)((PyModuleObject *)owner)->md_dict;
             assert(dict != NULL);
             DEOPT_IF(dict->ma_keys->dk_version != type_version, LOAD_ATTR);
diff --git a/Python/specialize.c b/Python/specialize.c
index 33a3c45..527b454 100644
--- a/Python/specialize.c
+++ b/Python/specialize.c
@@ -752,7 +752,7 @@ _Py_Specialize_LoadAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
         SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OTHER);
         goto fail;
     }
-    if (PyModule_CheckExact(owner)) {
+    if (PyModule_Check(owner)) {
         if (specialize_module_load_attr(owner, instr, name))
         {
             goto fail;
@@ -925,7 +925,7 @@ _Py_Specialize_StoreAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
         SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_OTHER);
         goto fail;
     }
-    if (PyModule_CheckExact(owner)) {
+    if (PyModule_Check(owner)) {
         SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_OVERRIDDEN);
         goto fail;
     }

Previous discussion

There are two ongoing discussions at the time of writing to make modules more class-like, here and here. While both of these proposals would benefit from this change, this change is generally useful. In fact, if implemented, it might be an argument for making the linked proposals external packages, and not language features.

Linked PRs

• gh-103951: enable optimization for fast attribute access on module subclasses #126264

[Fidget-Spinner]

Hi, thanks for taking a look at the specialised instruction for module attribute loading.

The general idea behind specialised instructions is that their guards must be cheap. PyModule_CheckExact is a single pointer comparison. PyModule_Check allows subyptes, so it will walk the entire MRO of the type just to check if it's a subtype of a module. This is generally significantly slower and the guard will thus become slower. This will especially negatively impact the guard failure case.

As such, I'm -1 on this change. The increase in specialisation coverage is not worth the significant slowdown in the fast path.

[ntessore]

Hi, I had considered this. Perhaps one could check just direct subclasses, Py_IS_TYPE((op), &PyModule_Type) || Py_IS_TYPE((op)->ob_type->tp_base, &PyModule_Type). This would allow the extra degree of freedom to achieve class-like modules.

[ntessore]

An alternative check could be more empirical and apply the specialisation for anything that uses the module tp_getattro, which should be a pretty exclusive club.

[skirpichev]

The general idea behind specialised instructions is that their guards must be cheap. PyModule_CheckExact is a single pointer comparison. PyModule_Check allows subyptes, so it will walk the entire MRO of the type just to check if it's a subtype of a module.

On another hand, _Py_Specialize_LoadAttr() uses PyType_Check() exactly for a similar purpose...

[Fidget-Spinner]

The general idea behind specialised instructions is that their guards must be cheap. PyModule_CheckExact is a single pointer comparison. PyModule_Check allows subyptes, so it will walk the entire MRO of the type just to check if it's a subtype of a module.

On another hand, _Py_Specialize_LoadAttr() uses PyType_Check() exactly for a similar purpose...

I assume you mean LOAD_ATTR specialisations, because it's fine for _Py_Specialize_LoadAttr to be slow, since the specialisation function is only called once every so often, while specialised instructions are the ones that are actually executed frequently.

PyType_Check is a simple bitwise & comparison with Py_TPFLAGS_TYPE_SUBCLASS. It's a fast constant time operation and does not require looking up the MRO. All subtypes of type set a special flag indicating they are so. Whereas module subtypes don't do that, so require traversing the MRO during PyModule_Check. We don't just add flags for everything because there's a limited number of bits reserved for these flags. So they're precious and only used when we really need to.

[ntessore]

Checking for the module type's tp_getattro isn't such a bad solution: anything with module's __getattr__ semantics is a candidate for the module specialisation. If you can stomach it.

Patch

diff --git a/Include/moduleobject.h b/Include/moduleobject.h
index 555564e..6cfdd30 100644
--- a/Include/moduleobject.h
+++ b/Include/moduleobject.h
@@ -101,6 +101,9 @@ struct PyModuleDef {
 // Internal C API
 #ifdef Py_BUILD_CORE
 extern int _PyModule_IsExtension(PyObject *obj);
+
+// Check if op uses the module attribute getter, used in specialization
+#define _PyModule_CheckGetAttr(op) (Py_TYPE(op)->tp_getattro == PyModule_Type.tp_getattro)
 #endif
 
 #ifdef __cplusplus
diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index 9de0d92..5acae62 100644
--- a/Python/bytecodes.c
+++ b/Python/bytecodes.c
@@ -1676,7 +1676,7 @@ dummy_func(
         }
 
         inst(LOAD_ATTR_MODULE, (unused/1, type_version/2, index/1, unused/5, owner -- res2 if (oparg & 1), res)) {
-            DEOPT_IF(!PyModule_CheckExact(owner), LOAD_ATTR);
+            DEOPT_IF(!_PyModule_CheckGetAttr(owner), LOAD_ATTR);
             PyDictObject *dict = (PyDictObject *)((PyModuleObject *)owner)->md_dict;
             assert(dict != NULL);
             DEOPT_IF(dict->ma_keys->dk_version != type_version, LOAD_ATTR);
diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h
index 864a4f7..67030e8 100644
--- a/Python/generated_cases.c.h
+++ b/Python/generated_cases.c.h
@@ -2338,7 +2338,7 @@
             uint32_t type_version = read_u32(&next_instr[1].cache);
             uint16_t index = read_u16(&next_instr[3].cache);
             #line 1679 "Python/bytecodes.c"
-            DEOPT_IF(!PyModule_CheckExact(owner), LOAD_ATTR);
+            DEOPT_IF(!_PyModule_CheckGetAttr(owner), LOAD_ATTR);
             PyDictObject *dict = (PyDictObject *)((PyModuleObject *)owner)->md_dict;
             assert(dict != NULL);
             DEOPT_IF(dict->ma_keys->dk_version != type_version, LOAD_ATTR);
diff --git a/Python/specialize.c b/Python/specialize.c
index 33a3c45..8c5d2d8 100644
--- a/Python/specialize.c
+++ b/Python/specialize.c
@@ -752,7 +752,7 @@ _Py_Specialize_LoadAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
         SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_OTHER);
         goto fail;
     }
-    if (PyModule_CheckExact(owner)) {
+    if (_PyModule_CheckGetAttr(owner)) {
         if (specialize_module_load_attr(owner, instr, name))
         {
             goto fail;
@@ -925,7 +925,7 @@ _Py_Specialize_StoreAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name)
         SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_OTHER);
         goto fail;
     }
-    if (PyModule_CheckExact(owner)) {
+    if (_PyModule_CheckGetAttr(owner)) {
         SPECIALIZATION_FAIL(STORE_ATTR, SPEC_FAIL_OVERRIDDEN);
         goto fail;
     }

[skirpichev]

@Fidget-Spinner, what do think on alternatives, presented by @ntessore?

[joaoe]

PEP 726 was rejected so this patch became much more relevant.
Thank you for your work.

Context:
https://discuss.python.org/t/pep-726-module-setattr-and-delattr/32640
https://peps.python.org/pep-0726/
Without the module __setattr__, when the only way to support that feature is to wrap the module with another object, hence this patch being relevant not to worsen performance.