GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Argument clinic: improve generated code for self converter type checks · Issue #101409 · python/cpython

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

New issue
Copy link
New issue
Copy link
Closed
Closed
Argument clinic: improve generated code for self converter type checks#101409
Copy link
Assignees
erlend-aasland
Labels
performancePerformance or resource usagePerformance or resource usagetopic-argument-clinictype-featureA feature request or enhancementA feature request or enhancement

Description

@erlend-aasland
erlend-aasland
opened on Jan 29, 2023
Issue body actions

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

  • gh-101409: Improve generated clinic code for self type checks #101411

Activity

  1. erlend-aasland
    added
    type-featureA feature request or enhancementA feature request or enhancement
    topic-argument-clinic
    on Jan 29, 2023
  2. erlend-aasland
    self-assigned this
    on Jan 29, 2023
  3. erlend-aasland
    added a commit that references this issue on Jan 29, 2023

    pythongh-101409: Improve generated clinic code for self type checks

    056abc4
  4. erlend-aasland commented on Jan 29, 2023

    @erlend-aasland
    erlend-aasland
    on Jan 29, 2023
    ContributorAuthor
    More actions

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

  5. mdboom
    added
    performancePerformance or resource usagePerformance or resource usage
    on Jan 30, 2023
  6. erlend-aasland
    added a commit that references this issue on Jan 31, 2023

    gh-101409: Improve generated clinic code for self type checks (#101411)

    2753cf2
  7. erlend-aasland
    closed this as completedon Jan 31, 2023
  8. carljm
    added a commit that references this issue on Jan 31, 2023

    Merge branch 'main' into inlinecomp2

    db208d5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

  • @erlend-aasland
    erlend-aasland

Labels

performancePerformance or resource usagePerformance or resource usagetopic-argument-clinictype-featureA feature request or enhancementA feature request or enhancement

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

    You are visitor 01402503 — GeezerVison does not store the pages you look up.
    Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.