Skip to content

bpo-40077: Convert _abc module to use PyType_FromSpec() #19202

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

Merged
merged 8 commits into from
Mar 30, 2020
Merged

bpo-40077: Convert _abc module to use PyType_FromSpec() #19202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
47eb700
bpo-40077: Convert _abc module to use PyType_FromSpec()
corona10 Mar 28, 2020
7756065
bpo-40077: rename
corona10 Mar 28, 2020
ec8ccc6
bpo-40077: Update test_abc.py
corona10 Mar 29, 2020
c2ade98
bpo-40077: Add comment for abc_invalidation_counter.
corona10 Mar 29, 2020
0c0a895
Update Modules/_abc.c
corona10 Mar 30, 2020
32eaf52
bpo-40077: Remove NEWS.d
corona10 Mar 30, 2020
98b9300
bpo-40077: Apply code review.
corona10 Mar 30, 2020
14c3e52
bpo-40077: Update the type of _abc_data_type.
corona10 Mar 30, 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: 1 addition & 1 deletion Lib/test/test_abc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ class B:
token_old = abc_get_cache_token()
A.register(B)
token_new = abc_get_cache_token()
self.assertNotEqual(token_old, token_new)
self.assertGreater(token_new, token_old)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test might be what we wanted exactly.
I just updated the test for more validation.

self.assertTrue(isinstance(b, A))
self.assertTrue(isinstance(b, (A,)))

Expand Down
105 changes: 75 additions & 30 deletions Modules/_abc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,26 @@ _Py_IDENTIFIER(_abc_impl);
_Py_IDENTIFIER(__subclasscheck__);
_Py_IDENTIFIER(__subclasshook__);

typedef struct {
PyTypeObject *_abc_data_type;
} _abcmodule_state;

/* A global counter that is incremented each time a class is
registered as a virtual subclass of anything. It forces the
negative cache to be cleared before its next use.
Note: this counter is private. Use `abc.get_cache_token()` for
external code. */
// FIXME: PEP 573: Move abc_invalidation_counter into _abcmodule_state.
static unsigned long long abc_invalidation_counter = 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that the abc_invalidation_counter is okay to be shared as static from interpreter Isolation point of view.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this into PyInterpreterState?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it would be safe to put it in the module state. The purpose of the counter is to invalidate the module cache. Registering a class in module instance 1 should not invalidate module instance 2. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I 've updated the PR.


static inline _abcmodule_state*
get_abc_state(PyObject *module)
{
void *state = PyModule_GetState(module);
assert(state != NULL);
return (_abcmodule_state *)state;
}

/* This object stores internal state for ABCs.
Note that we can use normal sets for caches,
since they are never iterated over. */
Expand All @@ -41,10 +54,12 @@ typedef struct {
static void
abc_data_dealloc(_abc_data *self)
{
PyTypeObject *tp = Py_TYPE(self);
Py_XDECREF(self->_abc_registry);
Py_XDECREF(self->_abc_cache);
Py_XDECREF(self->_abc_negative_cache);
Py_TYPE(self)->tp_free(self);
tp->tp_free(self);
Py_DECREF(tp);
}

static PyObject *
Expand All @@ -65,24 +80,29 @@ abc_data_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
PyDoc_STRVAR(abc_data_doc,
"Internal state held by ABC machinery.");

static PyTypeObject _abc_data_type = {
PyVarObject_HEAD_INIT(NULL, 0)
"_abc_data", /*tp_name*/
sizeof(_abc_data), /*tp_basicsize*/
.tp_dealloc = (destructor)abc_data_dealloc,
.tp_flags = Py_TPFLAGS_DEFAULT,
.tp_alloc = PyType_GenericAlloc,
.tp_new = abc_data_new,
static PyType_Slot _abc_data_type_spec_slots[] = {
{Py_tp_doc, (void *)abc_data_doc},
{Py_tp_new, abc_data_new},
{Py_tp_dealloc, abc_data_dealloc},
{0, 0}
};

static PyType_Spec _abc_data_type_spec = {
.name = "_abc._abc_data",
.basicsize = sizeof(_abc_data),
.flags = Py_TPFLAGS_DEFAULT,
.slots = _abc_data_type_spec_slots,
};

static _abc_data *
_get_impl(PyObject *self)
_get_impl(PyObject *module, PyObject *self)
{
_abcmodule_state *state = get_abc_state(module);
PyObject *impl = _PyObject_GetAttrId(self, &PyId__abc_impl);
if (impl == NULL) {
return NULL;
}
if (!Py_IS_TYPE(impl, &_abc_data_type)) {
if (!Py_IS_TYPE(impl, state->_abc_data_type)) {
PyErr_SetString(PyExc_TypeError, "_abc_impl is set to a wrong type");
Py_DECREF(impl);
return NULL;
Expand Down Expand Up @@ -179,7 +199,7 @@ static PyObject *
_abc__reset_registry(PyObject *module, PyObject *self)
/*[clinic end generated code: output=92d591a43566cc10 input=12a0b7eb339ac35c]*/
{
_abc_data *impl = _get_impl(self);
_abc_data *impl = _get_impl(module, self);
if (impl == NULL) {
return NULL;
}
Expand All @@ -206,7 +226,7 @@ static PyObject *
_abc__reset_caches(PyObject *module, PyObject *self)
/*[clinic end generated code: output=f296f0d5c513f80c input=c0ac616fd8acfb6f]*/
{
_abc_data *impl = _get_impl(self);
_abc_data *impl = _get_impl(module, self);
if (impl == NULL) {
return NULL;
}
Expand Down Expand Up @@ -241,7 +261,7 @@ static PyObject *
_abc__get_dump(PyObject *module, PyObject *self)
/*[clinic end generated code: output=9d9569a8e2c1c443 input=2c5deb1bfe9e3c79]*/
{
_abc_data *impl = _get_impl(self);
_abc_data *impl = _get_impl(module, self);
if (impl == NULL) {
return NULL;
}
Expand Down Expand Up @@ -391,13 +411,14 @@ static PyObject *
_abc__abc_init(PyObject *module, PyObject *self)
/*[clinic end generated code: output=594757375714cda1 input=8d7fe470ff77f029]*/
{
_abcmodule_state *state = get_abc_state(module);
PyObject *data;
if (compute_abstract_methods(self) < 0) {
return NULL;
}

/* Set up inheritance registry. */
data = abc_data_new(&_abc_data_type, NULL, NULL);
data = abc_data_new(state->_abc_data_type, NULL, NULL);
if (data == NULL) {
return NULL;
}
Expand Down Expand Up @@ -446,7 +467,7 @@ _abc__abc_register_impl(PyObject *module, PyObject *self, PyObject *subclass)
if (result < 0) {
return NULL;
}
_abc_data *impl = _get_impl(self);
_abc_data *impl = _get_impl(module, self);
if (impl == NULL) {
return NULL;
}
Expand Down Expand Up @@ -480,7 +501,7 @@ _abc__abc_instancecheck_impl(PyObject *module, PyObject *self,
/*[clinic end generated code: output=b8b5148f63b6b56f input=a4f4525679261084]*/
{
PyObject *subtype, *result = NULL, *subclass = NULL;
_abc_data *impl = _get_impl(self);
_abc_data *impl = _get_impl(module, self);
if (impl == NULL) {
return NULL;
}
Expand Down Expand Up @@ -576,7 +597,7 @@ _abc__abc_subclasscheck_impl(PyObject *module, PyObject *self,
PyObject *ok, *subclasses = NULL, *result = NULL;
Py_ssize_t pos;
int incache;
_abc_data *impl = _get_impl(self);
_abc_data *impl = _get_impl(module, self);
if (impl == NULL) {
return NULL;
}
Expand Down Expand Up @@ -795,7 +816,7 @@ _abc_get_cache_token_impl(PyObject *module)
return PyLong_FromUnsignedLongLong(abc_invalidation_counter);
}

static struct PyMethodDef module_functions[] = {
static struct PyMethodDef _abcmodule_methods[] = {
_ABC_GET_CACHE_TOKEN_METHODDEF
_ABC__ABC_INIT_METHODDEF
_ABC__RESET_REGISTRY_METHODDEF
Expand All @@ -808,30 +829,54 @@ static struct PyMethodDef module_functions[] = {
};

static int
_abc_exec(PyObject *module)
_abcmodule_exec(PyObject *module)
{
if (PyType_Ready(&_abc_data_type) < 0) {
_abcmodule_state *state = get_abc_state(module);
state->_abc_data_type = (PyTypeObject *)PyType_FromSpec(&_abc_data_type_spec);
if (state->_abc_data_type == NULL) {
return -1;
}
_abc_data_type.tp_doc = abc_data_doc;

return 0;
}

static int
_abcmodule_traverse(PyObject *module, visitproc visit, void *arg)
{
_abcmodule_state *state = get_abc_state(module);
Py_VISIT(state->_abc_data_type);
return 0;
}

static PyModuleDef_Slot _abc_slots[] = {
{Py_mod_exec, _abc_exec},
static int
_abcmodule_clear(PyObject *module)
{
_abcmodule_state *state = get_abc_state(module);
Py_CLEAR(state->_abc_data_type);
return 0;
}

static void
_abcmodule_free(void *module)
{
_abcmodule_clear((PyObject *)module);
}

static PyModuleDef_Slot _abcmodule_slots[] = {
{Py_mod_exec, _abcmodule_exec},
{0, NULL}
};

static struct PyModuleDef _abcmodule = {
PyModuleDef_HEAD_INIT,
"_abc",
_abc__doc__,
0,
module_functions,
_abc_slots,
NULL,
NULL,
NULL
sizeof(_abcmodule_state),
_abcmodule_methods,
_abcmodule_slots,
_abcmodule_traverse,
_abcmodule_clear,
_abcmodule_free,
};

PyMODINIT_FUNC
Expand Down