Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Argument clinic: improve generated code for self converter type checks #101409

Closed
erlend-aasland opened this issue Jan 29, 2023 · 1 comment
Closed

Argument clinic: improve generated code for self converter type checks #101409

erlend-aasland opened this issue Jan 29, 2023 · 1 comment
Assignees
@erlend-aasland
Labels
expert-argument-clinic performance Performance or resource usage type-feature A feature request or enhancement

Comments

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jan 29, 2023 •
edited

Feature or enhancement

Currently, typically for __new__ and __init__ methods, Argument Clinic will spell out the self type check as such:

if kind == METHOD_NEW:
    type_check = ('({0} == {1} ||\n        '
                  ' {0}->tp_init == {2}tp_init)'
                 ).format(self.name, type_object, prefix)
else:
    type_check = ('(Py_IS_TYPE({0}, {1}) ||\n        '
                  ' Py_TYPE({0})->tp_new == {2}tp_new)'
                 ).format(self.name, type_object, prefix)

prefix is a slightly modified variant of type_object, depending on if the latter is a pointer. This works swell in most cases, but with module state and heap types, the generated code is not optimal. First, let's just quote the AC docs on how to declare a class:

When you declare a class, you must also specify two aspects of its type in C: the type declaration you’d use for a pointer to an instance of this class, and a pointer to the PyTypeObject for this class.

For heap types with module state, you'd typically do something like this:

typedef struct {
   ...
} myclass_object;

typedef struct {
    PyTypeObject myclass_type;
} module_state;

static inline module_state *
find_state_by_type(PyTypeObject *tp)
{
    PyObject *mod = PyType_GetModuleByDef(&moduledef); // Potentially slow!
    void *state = PyModule_GetState(mod);
    return (module_state*)state;
}

#define clinic_state() (find_state_by_type(type))
#include "clinic/mymod.c.h"
#undef clinic_state

/*[clinic input]
module mymod
class mymod.myclass "myclass_object *" "clinic_state()->myclass_type"
[clinic start generated code]*/

Currently, this generates clinic code like this:

mymod_myclass_new(PyTypeObject *type, PyObject *args, PyObject *kwargs)
{
    ...

    if ((type == clinic_state()->RowType ||
         type->tp_init == clinic_state()->RowType->tp_init) &&

... potentially calling PyType_GetModuleByDef twice in the self type check.

Pitch

Suggesting to modify clinic to store the self type pointer in a local variable, and use that variable in the self type check. Proof-of-concept diff from the _sqlite extension module clinic code

diff --git a/Modules/_sqlite/clinic/cursor.c.h b/Modules/_sqlite/clinic/cursor.c.h
index 36b8d0051a..633ad2e73d 100644
--- a/Modules/_sqlite/clinic/cursor.c.h
+++ b/Modules/_sqlite/clinic/cursor.c.h
@@ -16,10 +16,11 @@ static int
 pysqlite_cursor_init(PyObject *self, PyObject *args, PyObject *kwargs)
 {
     int return_value = -1;
+    PyTypeObject *self_tp = clinic_state()->CursorType;
     pysqlite_Connection *connection;
 
-    if ((Py_IS_TYPE(self, clinic_state()->CursorType) ||
-         Py_TYPE(self)->tp_new == clinic_state()->CursorType->tp_new) &&
+    if ((Py_IS_TYPE(self, self_tp) ||
+         Py_TYPE(self)->tp_new == self_tp->tp_new) &&
         !_PyArg_NoKeywords("Cursor", kwargs)) {
         goto exit;
     }
@@ -318,4 +319,4 @@ pysqlite_cursor_close(pysqlite_Cursor *self, PyObject *Py_UNUSED(ignored))
 {
     return pysqlite_cursor_close_impl(self);
 }
-/*[clinic end generated code: output=e53e75a32a9d92bd input=a9049054013a1b77]*/
+/*[clinic end generated code: output=e5eac0cbe29c88ad input=a9049054013a1b77]*/

Previous discussion

#101302 (comment)

Linked PRs
@erlend-aasland erlend-aasland added type-feature A feature request or enhancement expert-argument-clinic labels Jan 29, 2023
@erlend-aasland erlend-aasland self-assigned this Jan 29, 2023
@bedevere-bot bedevere-bot mentioned this issue Jan 29, 2023
Merged
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jan 29, 2023
@erlend-aasland
pythongh-101409: Improve generated clinic code for self type checks
056abc4 
Store the self type pointer in a local variable.
@erlend-aasland
Copy link
Contributor Author

I'm not sure if this improvement is worth it, though; it may be churn for no benefit.

@mdboom mdboom added the performance Performance or resource usage label Jan 30, 2023
@erlend-aasland erlend-aasland mentioned this issue Jan 31, 2023
Merged
erlend-aasland added a commit that referenced this issue Jan 31, 2023
@erlend-aasland
gh-101409: Improve generated clinic code for self type checks (#101411)
2753cf2
carljm added a commit to carljm/cpython that referenced this issue Jan 31, 2023
@carljm
Merge branch 'main' into inlinecomp2
db208d5 
* main:
  pythonGH-100288: Skip extra work when failing to specialize LOAD_ATTR (pythonGH-101354)
  pythongh-101409: Improve generated clinic code for self type checks (python#101411)
  pythongh-98831: rewrite BEFORE_ASYNC_WITH and END_ASYNC_FOR in the instruction definition DSL (python#101458)
  pythongh-101469: Optimise get_io_state() by using _PyModule_GetState() (pythonGH-101470)
@erlend-aasland erlend-aasland mentioned this issue Jan 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@erlend-aasland erlend-aasland

Labels
expert-argument-clinic performance Performance or resource usage type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mdboom @erlend-aasland