-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-1635741 port _testbuffer to multi-phase init #22003
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
Conversation
Modules/_testbuffer.c
Outdated
m = PyModule_Create(&_testbuffermodule); | ||
if (m == NULL) | ||
return NULL; | ||
|
||
Py_SET_TYPE(&NDArray_Type, &PyType_Type); | ||
Py_INCREF(&NDArray_Type); | ||
PyModule_AddObject(m, "ndarray", (PyObject *)&NDArray_Type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can use PyModule_AddType
in here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shihai1991 yes and I guess I should convert these to heap types too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shihai1991 @vstinner I am trying to convert the NDArray_Type to a heap type but it has binary functions in its as_mapping that need access to the module state, e.g.:
nd = (NDArrayObject *)ndarray_new(&NDArray_Type, NULL, NULL);
The signature of this method is:
static PyObject *
ndarray_subscript(NDArrayObject *self, PyObject *key)
There doesn't seem to be a way to get the module in this case - so I believe converting to heap types might block until a future PEP. Please correct me if I am wrong but I will proceed with using PyModule_AddType without heap types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can take a look of victor's PR: 1c2fa78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shihai1991 That PR does not have any types with an as_mapping method defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is not possible to subclass the type, you can use PyType_GetModule().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vstinner I think I'm missing some information. How can I use PyType_GetModule()
on a method signature like:
static PyObject *
ndarray_subscript(NDArrayObject *self, PyObject *key)
There is no type here for me to access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Py_TYPE(self) gives you a type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vstinner oh, right. Every object struct starts with PyObject_HEAD which has the type. I totally forgot about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be simpler to have one PR for _testbuffer, and another for overlapped. These changes are really complex, so it's hard to review hard. It's simpler when a PR is on a single extension module. But you can also keep the PR as it is. It's up to you.
Modules/_testbuffer.c
Outdated
|
||
Struct = PyObject_GetAttrString(structmodule, "Struct"); | ||
calcsize = PyObject_GetAttrString(structmodule, "calcsize"); | ||
if (Struct == NULL || calcsize == NULL) | ||
return NULL; | ||
return -1; | ||
|
||
simple_format = PyUnicode_FromString(simple_fmt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, would it be possible to move these static variables to be able to clear them when the extension module is unloaded? It would require to pass the module to NDArray_Type creation, so be able to get the module state from a ndarray instance.
static const char *simple_fmt = "B";
static PyObject *simple_format = NULL;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vstinner This seems tricky. How do you recommend passing the module to a static type like NDArray_Type. I can't put the module instance on the type or it will be overridden each time we do _testbuffer_exec. Is there a way to do this in tp_new?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vstinner What if I just call PyUnicode_FromString every time it is used, since as you say this is just a test module. Struct and calcsize should be handled properly in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually to convert an extension module to multiphase init, you must convert static types to heap types first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vstinner see my comment in response to @shihai1991 about this - I was not able to convert these to heap types
Modules/_testbuffer.c
Outdated
PyMODINIT_FUNC | ||
PyInit__testbuffer(void) | ||
static int | ||
_testbuffer_exec(PyObject *m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting _testbuffer to multiphase init allows to create two instances of the module, but the C implementation uses global variables which is incompatible with that:
static PyObject *Struct = NULL;
static PyObject *calcsize = NULL;
You must first avoid these global variables. I suggest you to attempt to pass the module to functions using Struct and calcsize, and then get Struct and calcsize from the module. Either use PyObject_GetAttrString() which can slow down the module (which is ok, the module is only used for testing purpose), or use a module state (see my other comment about that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vstinner I am more comfortable with module state, anyways eventually it will need it if we migrate to heap types.
5ae8c40
to
3542f6a
Compare
I converted to heap types but now tests are failing (seems to be due to the "mismatch between initializer element and format string" exception thrown by pack_single). I'm going to try to figure out what I did that causes this. |
Good luck :-) Ping me when you consider that the PR is ready for a new review. You can write a first PR just to convert static types to heap types if you prefer, to ease debug, and to ease review. |
Same here: It is a test module that does not require changes. |
@vstinner @shihai1991 please review
This is the other half of #21986
https://bugs.python.org/issue1635741