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

gh-71587: Isolate _datetime #102995

Closed
wants to merge 33 commits into from

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Mar 24, 2023

@erlend-aasland erlend-aasland changed the title [PoC] Isolate _datetime gh-71587: Isolate _datetime Mar 24, 2023
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Mar 24, 2023

Observations:

  • the (fishy) time and datetime memory optimisations may need to go; for now, I tore them out
  • it may be beneficial to store state in object context instead of passing it around
  • the exposed C API is still using a global variable; this is unfortunate

@erlend-aasland
Copy link
Contributor Author

@ericsnowcurrently, regarding the datetime C API:

Currently, the encapsulated datetime C API is exposed as a global variable:

cpython/Include/datetime.h

Lines 196 to 197 in e375bff

/* Define global variable for the C API and a macro for setting it. */
static PyDateTime_CAPI *PyDateTimeAPI = NULL;

I guess we could move this to the interpreter state instead. Thoughts?

FTR, if we run the ref leak bots on this PR, they fail because of the datetime C API tests in testcapi:

static PyObject *
test_datetime_capi(PyObject *self, PyObject *args)
{
if (PyDateTimeAPI) {
if (test_run_counter) {
/* Probably regrtest.py -R */
Py_RETURN_NONE;
}
else {
PyErr_SetString(PyExc_AssertionError,
"PyDateTime_CAPI somehow initialized");
return NULL;
}
}
test_run_counter++;
PyDateTime_IMPORT;
if (PyDateTimeAPI) {
Py_RETURN_NONE;
}
return NULL;
}

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Would it be possible to split the PR into multiple parts? Example:

  • PR 1: Add a module state, refer a few static types there, and pass the state to the easy places to retrieve the type
  • PR 2: Slowly, convert static types, one by one
  • PR 3: Dirty changes
  • PR 4: The final beautiful change which just remove the old code

@erlend-aasland
Copy link
Contributor Author

Would it be possible to split the PR into multiple parts? Example:

  • PR 1: Add a module state, refer a few static types there, and pass the state to the easy places to retrieve the type
  • PR 2: Slowly, convert static types, one by one
  • PR 3: Dirty changes
  • PR 4: The final beautiful change which just remove the old code

Definitely!

Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Show resolved Hide resolved
Modules/_datetimemodule.c Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
@@ -6830,7 +6897,7 @@ _datetime_exec(PyObject *module)
return -1;
}

PyDateTime_CAPI *capi = get_datetime_capi();
PyDateTime_CAPI *capi = get_datetime_capi(st);
Copy link
Member

Choose a reason for hiding this comment

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

This function name is misleading. Can you please rename it to create_datetime_capi()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but I think we should do that in a separate PR. The get_datetime_capi is not introduced by this PR; it is already in place.

Modules/_datetimemodule.c Outdated Show resolved Hide resolved
.basicsize = sizeof(PyDateTime_DateTime),
.flags = (Py_TPFLAGS_DEFAULT |
Py_TPFLAGS_BASETYPE |
Py_TPFLAGS_HAVE_GC |
Copy link
Member

Choose a reason for hiding this comment

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

Something is strange here. But you should better address it in a separated PR (to keep this one as small as possible). Apparently, PyObject_GC_Track() is simply never called and so Py_TPFLAGS_HAVE_GC, traverse() and clear() functions seem to alll be unused (useless).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants