Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commented Mar 24, 2023

@erlend-aaslanderlend-aasland changed the title [PoC] Isolate _datetimegh-71587: Isolate _datetimeMar 24, 2023
@erlend-aasland
Copy link
ContributorAuthor

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
ContributorAuthor

@ericsnowcurrently, regarding the datetime C API:

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

/* Define global variable for the C API and a macro for setting it. */
staticPyDateTime_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:

staticPyObject*
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");
returnNULL;
}
}
test_run_counter++;
PyDateTime_IMPORT;
if (PyDateTimeAPI){
Py_RETURN_NONE;
}
returnNULL;
}

Copy link
Member

@vstinnervstinner 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
ContributorAuthor

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!

@@ -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
ContributorAuthor

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.

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

@erlend-aasland@vstinner@bedevere-bot