From 04b23813ffa879bebb99739e997845f3fdeefe7e Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Mon, 23 Sep 2024 08:37:40 +0900 Subject: [PATCH 1/6] abandon if-else chains for maintainability --- Objects/typeobject.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 68e481f8e5163b..adab466f546504 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5340,24 +5340,23 @@ get_base_by_token_from_mro(PyTypeObject *type, void *token) static int check_base_by_token(PyTypeObject *type, void *token) { - // Chain the branches, which will be optimized exclusive here if (token == NULL) { PyErr_Format(PyExc_SystemError, "PyType_GetBaseByToken called with token=NULL"); return -1; } - else if (!PyType_Check(type)) { + if (!PyType_Check(type)) { PyErr_Format(PyExc_TypeError, "expected a type, got a '%T' object", type); return -1; } - else if (!_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { + if (!_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { return 0; } - else if (((PyHeapTypeObject*)type)->ht_token == token) { + if (((PyHeapTypeObject*)type)->ht_token == token) { return 1; } - else if (type->tp_mro != NULL) { + if (type->tp_mro != NULL) { // This will not be inlined return get_base_by_token_from_mro(type, token) ? 1 : 0; } @@ -5379,19 +5378,19 @@ PyType_GetBaseByToken(PyTypeObject *type, void *token, PyTypeObject **result) return check_base_by_token(type, token); } - // Chain the branches, which will be optimized exclusive here - PyTypeObject *base; if (!_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { // No static type has a heaptype superclass, // which is ensured by type_ready_mro(). *result = NULL; return 0; } - else if (((PyHeapTypeObject*)type)->ht_token == token) { + if (((PyHeapTypeObject*)type)->ht_token == token) { *result = (PyTypeObject *)Py_NewRef(type); return 1; } - else if (type->tp_mro != NULL) { + + PyTypeObject *base; + if (type->tp_mro != NULL) { // Expect this to be inlined base = get_base_by_token_from_mro(type, token); } From c2cabd1fed216a600fe028b8cc0bb9b7d71b8e82 Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Mon, 23 Sep 2024 08:44:22 +0900 Subject: [PATCH 2/6] add forward declarations The order might have a very slight effect. --- Objects/typeobject.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index adab466f546504..6b9784ff9e56c7 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5288,6 +5288,18 @@ _PyType_GetModuleByDef2(PyTypeObject *left, PyTypeObject *right, } +// PyType_GetBaseByToken() family are better to be placed from most to least +// frequent in the src and obj files (the decls in random order) for PGO on +// MSVC. They'll be optimized not for speed if a cold sub function precedes, +// in which case the unrelated functions above can be slower if there are +// common conds such as `if (!_PyType_HasFeature(tp, Py_TPFLAGS_HEAPTYPE))`. +static int +check_base_by_token(PyTypeObject *, void *); +static inline PyTypeObject * +get_base_by_token_from_mro(PyTypeObject *, void *); +static PyTypeObject * +get_base_by_token_recursive(PyTypeObject *, void *); + static PyTypeObject * get_base_by_token_recursive(PyTypeObject *type, void *token) { From ead4411232161a65222887ebdfc4c8b14e05c19b Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Mon, 23 Sep 2024 08:49:40 +0900 Subject: [PATCH 3/6] move down get_base_by_token_from_mro() --- Objects/typeobject.c | 54 ++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 6b9784ff9e56c7..831532539d903e 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5323,33 +5323,6 @@ get_base_by_token_recursive(PyTypeObject *type, void *token) return NULL; } -static inline PyTypeObject * -get_base_by_token_from_mro(PyTypeObject *type, void *token) -{ - // Bypass lookup_tp_mro() as PyType_IsSubtype() does - PyObject *mro = type->tp_mro; - assert(mro != NULL); - assert(PyTuple_Check(mro)); - // mro_invoke() ensures that the type MRO cannot be empty. - assert(PyTuple_GET_SIZE(mro) >= 1); - // Also, the first item in the MRO is the type itself, which is supposed - // to be already checked by the caller. We skip it in the loop. - assert(PyTuple_GET_ITEM(mro, 0) == (PyObject *)type); - assert(PyType_GetSlot(type, Py_tp_token) != token); - - Py_ssize_t n = PyTuple_GET_SIZE(mro); - for (Py_ssize_t i = 1; i < n; i++) { - PyTypeObject *base = _PyType_CAST(PyTuple_GET_ITEM(mro, i)); - if (!_PyType_HasFeature(base, Py_TPFLAGS_HEAPTYPE)) { - continue; - } - if (((PyHeapTypeObject*)base)->ht_token == token) { - return base; - } - } - return NULL; -} - static int check_base_by_token(PyTypeObject *type, void *token) { if (token == NULL) { @@ -5420,6 +5393,33 @@ PyType_GetBaseByToken(PyTypeObject *type, void *token, PyTypeObject **result) } } +static inline PyTypeObject * +get_base_by_token_from_mro(PyTypeObject *type, void *token) +{ + // Bypass lookup_tp_mro() as PyType_IsSubtype() does + PyObject *mro = type->tp_mro; + assert(mro != NULL); + assert(PyTuple_Check(mro)); + // mro_invoke() ensures that the type MRO cannot be empty. + assert(PyTuple_GET_SIZE(mro) >= 1); + // Also, the first item in the MRO is the type itself, which is supposed + // to be already checked by the caller. We skip it in the loop. + assert(PyTuple_GET_ITEM(mro, 0) == (PyObject *)type); + assert(PyType_GetSlot(type, Py_tp_token) != token); + + Py_ssize_t n = PyTuple_GET_SIZE(mro); + for (Py_ssize_t i = 1; i < n; i++) { + PyTypeObject *base = _PyType_CAST(PyTuple_GET_ITEM(mro, i)); + if (!_PyType_HasFeature(base, Py_TPFLAGS_HEAPTYPE)) { + continue; + } + if (((PyHeapTypeObject*)base)->ht_token == token) { + return base; + } + } + return NULL; +} + void * PyObject_GetTypeData(PyObject *obj, PyTypeObject *cls) From 7349f97bd47e0f5e54cd15ae6848169daa57a665 Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Mon, 23 Sep 2024 08:51:14 +0900 Subject: [PATCH 4/6] move down check_base_by_token() --- Objects/typeobject.c | 54 ++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 831532539d903e..35051581f285a3 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5323,33 +5323,6 @@ get_base_by_token_recursive(PyTypeObject *type, void *token) return NULL; } -static int -check_base_by_token(PyTypeObject *type, void *token) { - if (token == NULL) { - PyErr_Format(PyExc_SystemError, - "PyType_GetBaseByToken called with token=NULL"); - return -1; - } - if (!PyType_Check(type)) { - PyErr_Format(PyExc_TypeError, - "expected a type, got a '%T' object", type); - return -1; - } - if (!_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { - return 0; - } - if (((PyHeapTypeObject*)type)->ht_token == token) { - return 1; - } - if (type->tp_mro != NULL) { - // This will not be inlined - return get_base_by_token_from_mro(type, token) ? 1 : 0; - } - else { - return get_base_by_token_recursive(type, token) ? 1 : 0; - } -} - int PyType_GetBaseByToken(PyTypeObject *type, void *token, PyTypeObject **result) { @@ -5420,6 +5393,33 @@ get_base_by_token_from_mro(PyTypeObject *type, void *token) return NULL; } +static int +check_base_by_token(PyTypeObject *type, void *token) { + if (token == NULL) { + PyErr_Format(PyExc_SystemError, + "PyType_GetBaseByToken called with token=NULL"); + return -1; + } + if (!PyType_Check(type)) { + PyErr_Format(PyExc_TypeError, + "expected a type, got a '%T' object", type); + return -1; + } + if (!_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { + return 0; + } + if (((PyHeapTypeObject*)type)->ht_token == token) { + return 1; + } + if (type->tp_mro != NULL) { + // This will not be inlined + return get_base_by_token_from_mro(type, token) ? 1 : 0; + } + else { + return get_base_by_token_recursive(type, token) ? 1 : 0; + } +} + void * PyObject_GetTypeData(PyObject *obj, PyTypeObject *cls) From f938abc70384eb7773a77faec26a4a5225c82d63 Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Mon, 23 Sep 2024 08:52:52 +0900 Subject: [PATCH 5/6] move down get_base_by_token_recursive() --- Objects/typeobject.c | 47 ++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 35051581f285a3..6b031207df99de 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5300,29 +5300,6 @@ get_base_by_token_from_mro(PyTypeObject *, void *); static PyTypeObject * get_base_by_token_recursive(PyTypeObject *, void *); -static PyTypeObject * -get_base_by_token_recursive(PyTypeObject *type, void *token) -{ - assert(PyType_GetSlot(type, Py_tp_token) != token); - PyObject *bases = lookup_tp_bases(type); - assert(bases != NULL); - Py_ssize_t n = PyTuple_GET_SIZE(bases); - for (Py_ssize_t i = 0; i < n; i++) { - PyTypeObject *base = _PyType_CAST(PyTuple_GET_ITEM(bases, i)); - if (!_PyType_HasFeature(base, Py_TPFLAGS_HEAPTYPE)) { - continue; - } - if (((PyHeapTypeObject*)base)->ht_token == token) { - return base; - } - base = get_base_by_token_recursive(base, token); - if (base != NULL) { - return base; - } - } - return NULL; -} - int PyType_GetBaseByToken(PyTypeObject *type, void *token, PyTypeObject **result) { @@ -5355,7 +5332,6 @@ PyType_GetBaseByToken(PyTypeObject *type, void *token, PyTypeObject **result) else { base = get_base_by_token_recursive(type, token); } - if (base != NULL) { *result = (PyTypeObject *)Py_NewRef(base); return 1; @@ -5420,6 +5396,29 @@ check_base_by_token(PyTypeObject *type, void *token) { } } +static PyTypeObject * +get_base_by_token_recursive(PyTypeObject *type, void *token) +{ + assert(PyType_GetSlot(type, Py_tp_token) != token); + PyObject *bases = lookup_tp_bases(type); + assert(bases != NULL); + Py_ssize_t n = PyTuple_GET_SIZE(bases); + for (Py_ssize_t i = 0; i < n; i++) { + PyTypeObject *base = _PyType_CAST(PyTuple_GET_ITEM(bases, i)); + if (!_PyType_HasFeature(base, Py_TPFLAGS_HEAPTYPE)) { + continue; + } + if (((PyHeapTypeObject*)base)->ht_token == token) { + return base; + } + base = get_base_by_token_recursive(base, token); + if (base != NULL) { + return base; + } + } + return NULL; +} + void * PyObject_GetTypeData(PyObject *obj, PyTypeObject *cls) From 516d2fb8e319e807c004aac8672200a23bf499cf Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Mon, 23 Sep 2024 10:32:25 +0900 Subject: [PATCH 6/6] add again a comment about gotos --- Objects/typeobject.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 6b031207df99de..cfca6767f703b3 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5303,6 +5303,7 @@ get_base_by_token_recursive(PyTypeObject *, void *); int PyType_GetBaseByToken(PyTypeObject *type, void *token, PyTypeObject **result) { + // Goto jumps here can disturb the PGO collection on MSVC if (result == NULL) { // If the `result` is checked only once here, the subsequent // branches will become trivial to optimize.