Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

bpo-1635741 port _testbuffer to multi-phase init #22003

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ec3c86e
port _testbuffer to multi-phase
koubaa Aug 11, 2020
3542f6a
blurb
koubaa Sep 1, 2020
64d4c62
test module state
koubaa Sep 2, 2020
a1db5b3
heap types
koubaa Sep 6, 2020
f269a43
really switch to heap types
koubaa Sep 6, 2020
282a416
fix issue
koubaa Sep 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2020-09-01-17-24-34.bpo-1635741.WTmuDL.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Port the :mod:`_testbuffer` extension module to multi-phase initialization
(:pep:`489`).
59 changes: 28 additions & 31 deletions Modules/_testbuffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2813,48 +2813,29 @@ static struct PyMethodDef _testbuffer_functions[] = {
{NULL, NULL}
};

static struct PyModuleDef _testbuffermodule = {
PyModuleDef_HEAD_INIT,
"_testbuffer",
NULL,
-1,
_testbuffer_functions,
NULL,
NULL,
NULL,
NULL
};


PyMODINIT_FUNC
PyInit__testbuffer(void)
static int
_testbuffer_exec(PyObject *m)
Copy link
Member

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).

Copy link
Contributor Author

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.

{
PyObject *m;

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);
if (PyModule_AddType(m, &NDArray_Type) < 0) {
return -1;
}

Py_SET_TYPE(&StaticArray_Type, &PyType_Type);
Py_INCREF(&StaticArray_Type);
PyModule_AddObject(m, "staticarray", (PyObject *)&StaticArray_Type);
if (PyModule_AddType(m, &StaticArray_Type) < 0) {
return -1;
}

structmodule = PyImport_ImportModule("struct");
if (structmodule == NULL)
return NULL;
return -1;

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);
Copy link
Member

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;

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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

if (simple_format == NULL)
return NULL;
return -1;

PyModule_AddIntMacro(m, ND_MAX_NDIM);
PyModule_AddIntMacro(m, ND_VAREXPORT);
Expand Down Expand Up @@ -2887,8 +2868,24 @@ PyInit__testbuffer(void)
PyModule_AddIntMacro(m, PyBUF_READ);
PyModule_AddIntMacro(m, PyBUF_WRITE);

return m;
return 0;
}

static PyModuleDef_Slot _testbuffer_slots[] = {
{Py_mod_exec, _testbuffer_exec},
{0, NULL}
};

static struct PyModuleDef _testbuffermodule = {
PyModuleDef_HEAD_INIT,
.m_name = "_testbuffer",
.m_size = 0,
.m_methods = _testbuffer_functions,
.m_slots = _testbuffer_slots
};

PyMODINIT_FUNC
PyInit__testbuffer(void)
{
return PyModuleDef_Init(&_testbuffermodule);
}