gh-117398: Add datetime Module State #119810

ericsnowcurrently · 2024-05-30T21:08:30Z

I was able to make use of the existing datetime_state struct, but there was one tricky thing I had to sort out. We mostly aren't converting to heap types, so we can't use things like PyType_GetModuleByDef() to look up the module state. The solution I came up with is somewhat novel, but I consider it straightforward. Also, it shouldn't have much impact on performance.

In summary, this main changes of this PR are:

I've added some macros to help hide how various objects relate to module state
as a solution to the module state lookup problem, I've stored the last loaded module on the current interpreter's internal dict (actually a weakref)
if the static type method is used after the module has been deleted, it is reloaded
to avoid extra work when loading the module, we directly copy the objects (new refs only) from the old module state into the new state if the old module hasn't been deleted yet
during module init we set various objects on the static types' __dict__s; to simplify things, we only do that the first time; once those static types have a separate __dict__ per interpreter, we'll do it every time
we now clear the module state when the module is destroyed (before, we were leaking everything in _datetime_global_state)

Issue: Isolate the _datetime extension module #117398

ericsnowcurrently · 2024-05-30T21:09:16Z

~~FYI, I'm still tracking down a small refleak. I hope to have that figured out soon.~~

ericsnowcurrently

Here are some extra notes about the implementation here.

Modules/_datetimemodule.c

neonene

I tried adding attribute getters to the IsoCalendarDate type to see the performance of GET_CURRENT_STATE():

from timeit import timeit
cmd = 'import _datetime; C = _datetime.date.today().isocalendar()'
print('base:', timeit('C.base', cmd))  # PyType_GetModuleByDef()
print('test:', timeit('C.test', cmd))  # GET_CURRENT_STATE()

Windows non-debug build:

base: 0.0286023854278028  # PyType_GetModuleByDef()

test: 0.0937444182112813 3.27x  # INTERP_KEY: "cached-datetime-module"
test: 0.0406443682964891 1.42x  # INTERP_KEY: &_Py_ID(cached_datetime_module)

getset implementation

static PyObject *
bench_base(PyDateTime_IsoCalendarDate *self, void *unused)
{
    PyObject *mod = PyType_GetModuleByDef(Py_TYPE(self), &datetimemodule);
    datetime_state *st = get_module_state(mod);
    Py_RETURN_NONE;
}

static PyObject *
bench_test(PyDateTime_IsoCalendarDate *self, void *unused)
{
    datetime_state *st = GET_CURRENT_STATE(st);
    RELEASE_CURRENT_STATE(st);
    Py_RETURN_NONE;
}

static PyGetSetDef iso_calendar_date_getset[] = {
    {"base", (getter)bench_base},
    {"test", (getter)bench_test},
    {NULL}
};

neonene · 2024-05-31T11:10:44Z

Modules/_datetimemodule.c

 #define DATETIME_ADD_MACRO(dict, c, value_expr)         \
    do {                                                \
+      if (PyDict_GetItemString(dict, c) == NULL) {      \


This macro needs Py_DECREF(value_expr) if the condition is not true.

The expression isn't evaluated until the second statement inside this branch. Perhaps I've missed something?

#define DATETIME_ADD_MACRO(dict, c, value_expr) \ do { \ if (PyDict_GetItemString(dict, c) == NULL) { \ ... \ Py_DECREF(value); \ } \ + else { \ + Py_DECREF(value_expr); \ + } \ } while(0)

If this example fixes the refleaks test, then the leak comes from:

cpython/Modules/_datetimemodule.c

Lines 7173 to 7179 in 3de1cd3

/* -23:59 */

PyObject *min = create_timezone_from_delta(-1, 60, 0, 1);

DATETIME_ADD_MACRO(d, "min", min);

/* +23:59 */

PyObject *max = create_timezone_from_delta(0, (23 * 60 + 59) * 60, 0, 0);

DATETIME_ADD_MACRO(d, "max", max);

Ah, I understand now. Yeah, fixing that does resolve the refleak. Thanks!

neonene · 2024-05-31T11:13:49Z

Modules/_datetimemodule.c

+copy_state(datetime_state *st, datetime_state *from)
+{
+    *st = (datetime_state){
+        .isocalendar_date_type
+                = (PyTypeObject *)Py_NewRef(from->isocalendar_date_type),


The ht_module field of the heaptype needs to be updated here to keep the module alive even after del sys.modules['_datetime']?

Good point. It would probably be best to create a new type object in this case rather than add a ref.

Do you think this type is easier to maintain if it is static as well?

Perhaps. However, I'd rather avoid using static types for a multi-phase init module if possible.

ericsnowcurrently · 2024-05-31T20:02:52Z

Thanks for the review, @neonene! I have everything sorted out and the free-threaded/WASI builds should be passing now.

neonene · 2024-06-01T09:45:15Z

Does it make sense for this PR to drop the no_rerun decorator in datetimetester.py?

ericsnowcurrently · 2024-06-01T18:16:46Z

Good question. I'll take a look.

ericsnowcurrently · 2024-06-01T22:53:31Z

Let's not block this PR on that. We can drop @no_rerun separately. I tried it really quickly and there were no refleaks, but the same is true of main. We can figure it out later though.

encukou

This is getting out of hand; I con't stop thinking of the suggestion in that other thread about allowing immortal singleton modules.
I'm also reminded of the single-phase-init behaviour of copying __dict__ contents to reuse types/singletons...

On the other hand, if this is the last (so, presumably, most complicated) stdlib module that needs to support isolated state, I guess things aren't all that bad.

encukou · 2024-06-03T11:45:27Z

Modules/_datetimemodule.c

-#define PyTZInfo_Check(op) PyObject_TypeCheck(op, get_datetime_state()->tzinfo_type)
-#define PyTZInfo_CheckExact(op) Py_IS_TYPE(op, get_datetime_state()->tzinfo_type)
+#define GET_CURRENT_STATE(ST_VAR)   \
+    NULL;                           \


It's there so the macro can transparently declare the current_mod local var. However, that made more sense in an earlier iteration of the PR. I'll simplify this for clarity.

Modules/_datetimemodule.c

ericsnowcurrently · 2024-06-03T17:10:09Z

This is getting out of hand; I con't stop thinking of the suggestion in that other thread about allowing immortal singleton modules. I'm also reminded of the single-phase-init behaviour of copying __dict__ contents to reuse types/singletons...

On the other hand, if this is the last (so, presumably, most complicated) stdlib module that needs to support isolated state, I guess things aren't all that bad.

Yeah, this is the last one and the only one with the complexity of its own C-API (which exposes objects directly).

FWIW, I'm not especially satisfied with the "current module" stuff here, but consider it to be a straight-forward solution for this immediate, isolated situation. I expect we can find a better approach for 3.14+, but didn't want to take those sorts of risks in 3.13.

ericsnowcurrently · 2024-06-03T18:00:00Z

Any objections to merging this PR at this point?

ericsnowcurrently · 2024-06-03T21:54:38Z

In the interest of landing this for beta 2, I'm going to merge this now. I'm happy to apply tweaks in a follow-up PR.

miss-islington-app · 2024-06-03T21:56:03Z

Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

I was able to make use of the existing datetime_state struct, but there was one tricky thing I had to sort out. We mostly aren't converting to heap types, so we can't use things like PyType_GetModuleByDef() to look up the module state. The solution I came up with is somewhat novel, but I consider it straightforward. Also, it shouldn't have much impact on performance. In summary, this main changes here are: * I've added some macros to help hide how various objects relate to module state * as a solution to the module state lookup problem, I've stored the last loaded module on the current interpreter's internal dict (actually a weakref) * if the static type method is used after the module has been deleted, it is reloaded * to avoid extra work when loading the module, we directly copy the objects (new refs only) from the old module state into the new state if the old module hasn't been deleted yet * during module init we set various objects on the static types' __dict__s; to simplify things, we only do that the first time; once those static types have a separate __dict__ per interpreter, we'll do it every time * we now clear the module state when the module is destroyed (before, we were leaking everything in _datetime_global_state) (cherry picked from commit d82a7ba) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

bedevere-app · 2024-06-03T21:56:12Z

GH-120004 is a backport of this pull request to the 3.13 branch.

I was able to make use of the existing datetime_state struct, but there was one tricky thing I had to sort out. We mostly aren't converting to heap types, so we can't use things like PyType_GetModuleByDef() to look up the module state. The solution I came up with is somewhat novel, but I consider it straightforward. Also, it shouldn't have much impact on performance. In summary, this main changes here are: * I've added some macros to help hide how various objects relate to module state * as a solution to the module state lookup problem, I've stored the last loaded module on the current interpreter's internal dict (actually a weakref) * if the static type method is used after the module has been deleted, it is reloaded * to avoid extra work when loading the module, we directly copy the objects (new refs only) from the old module state into the new state if the old module hasn't been deleted yet * during module init we set various objects on the static types' __dict__s; to simplify things, we only do that the first time; once those static types have a separate __dict__ per interpreter, we'll do it every time * we now clear the module state when the module is destroyed (before, we were leaking everything in _datetime_global_state)

I was able to make use of the existing datetime_state struct, but there was one tricky thing I had to sort out. We mostly aren't converting to heap types, so we can't use things like PyType_GetModuleByDef() to look up the module state. The solution I came up with is somewhat novel, but I consider it straightforward. Also, it shouldn't have much impact on performance. In summary, this main changes here are: * I've added some macros to help hide how various objects relate to module state * as a solution to the module state lookup problem, I've stored the last loaded module on the current interpreter's internal dict (actually a weakref) * if the static type method is used after the module has been deleted, it is reloaded * to avoid extra work when loading the module, we directly copy the objects (new refs only) from the old module state into the new state if the old module hasn't been deleted yet * during module init we set various objects on the static types' __dict__s; to simplify things, we only do that the first time; once those static types have a separate __dict__ per interpreter, we'll do it every time * we now clear the module state when the module is destroyed (before, we were leaking everything in _datetime_global_state) (cherry picked from commit d82a7ba, AKA gh-119810) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

I was able to make use of the existing datetime_state struct, but there was one tricky thing I had to sort out. We mostly aren't converting to heap types, so we can't use things like PyType_GetModuleByDef() to look up the module state. The solution I came up with is somewhat novel, but I consider it straightforward. Also, it shouldn't have much impact on performance. In summary, this main changes here are: * I've added some macros to help hide how various objects relate to module state * as a solution to the module state lookup problem, I've stored the last loaded module on the current interpreter's internal dict (actually a weakref) * if the static type method is used after the module has been deleted, it is reloaded * to avoid extra work when loading the module, we directly copy the objects (new refs only) from the old module state into the new state if the old module hasn't been deleted yet * during module init we set various objects on the static types' __dict__s; to simplify things, we only do that the first time; once those static types have a separate __dict__ per interpreter, we'll do it every time * we now clear the module state when the module is destroyed (before, we were leaking everything in _datetime_global_state)

ericsnowcurrently added 14 commits May 29, 2024 10:37

Use the static types directly.

8463ae8

Use the UTC singleton directly.

ead0083

Add module state.

4c52d5d

Clear the module state when destroying the module.

6b291c8

Create PyDateTime_IsoCalendarDateType in init_state().

69c7e1f

Track the "current" module in the internal per-interpreter dict.

36dbeea

Copy the "current" module state, if available.

d0c0b2d

Make state usage explicit.

0599dd6

Reduce churn.

1f1f6fc

Drop _datetime_global_state.

d44d6e9

Drop datetime_state.initialized.

a309474

Fix refleaks.

ede4415

Clear the "current" module when finalizing the module.

5a8b1aa

Use a weakref when tracking the "current" module.

3de1cd3

ericsnowcurrently added the skip news label May 30, 2024

ericsnowcurrently requested a review from erlend-aasland May 30, 2024 21:08

ericsnowcurrently requested review from pganssle and abalkin as code owners May 30, 2024 21:08

bedevere-app bot added the awaiting core review label May 30, 2024

bedevere-app bot mentioned this pull request May 30, 2024

Isolate the _datetime extension module #117398

Closed

ericsnowcurrently mentioned this pull request May 30, 2024

gh-117398: Add datetime Module State #119640

Closed

ericsnowcurrently commented May 30, 2024

View reviewed changes

Modules/_datetimemodule.c Outdated Show resolved Hide resolved

Modules/_datetimemodule.c 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

ericsnowcurrently requested a review from encukou May 30, 2024 21:32

neonene reviewed May 31, 2024

View reviewed changes

ericsnowcurrently added 5 commits May 31, 2024 10:32

Fix clear_current_module().

62b3d5e

Give each module its own heap types.

5c25927

Consolidate into init_state().

da24674

Use _Py_ID() for INTERP_KEY.

f8420ea

Handle PyWeakref_GetRef() returning None.

2a2c0b1

ericsnowcurrently added 3 commits May 31, 2024 12:11

Fix refleak.

c519e3c

Make sure the module exists in static type methods.

05acc56

Don't bother making the module temporary.

07e3b65

ericsnowcurrently mentioned this pull request Jun 1, 2024

gh-117398: Use Per-Interpreter State for the _datetime Static Types #119929

Merged

encukou reviewed Jun 3, 2024

View reviewed changes

ericsnowcurrently added 2 commits June 3, 2024 11:31

Add a comment about the macros.

dc4b458

Simplify GET_CURRENT_STATE().

3c55035

ericsnowcurrently added the needs backport to 3.13 bugs and security fixes label Jun 3, 2024

ericsnowcurrently merged commit d82a7ba into python:main Jun 3, 2024
35 checks passed

bedevere-app bot removed the awaiting core review label Jun 3, 2024

ericsnowcurrently deleted the datetime-module-state-2 branch June 3, 2024 21:56

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-117398: Add datetime Module State #119810

gh-117398: Add datetime Module State #119810

ericsnowcurrently commented May 30, 2024 •

edited

Loading

ericsnowcurrently commented May 30, 2024 •

edited

Loading

ericsnowcurrently left a comment

neonene left a comment

neonene May 31, 2024

ericsnowcurrently May 31, 2024

neonene May 31, 2024 •

edited

Loading

ericsnowcurrently May 31, 2024

ericsnowcurrently Jun 3, 2024

neonene May 31, 2024

ericsnowcurrently May 31, 2024

neonene May 31, 2024

ericsnowcurrently May 31, 2024

ericsnowcurrently commented May 31, 2024

neonene commented Jun 1, 2024

ericsnowcurrently commented Jun 1, 2024

ericsnowcurrently commented Jun 1, 2024

encukou left a comment

encukou Jun 3, 2024

ericsnowcurrently Jun 3, 2024

ericsnowcurrently Jun 3, 2024

ericsnowcurrently commented Jun 3, 2024

ericsnowcurrently commented Jun 3, 2024

ericsnowcurrently commented Jun 3, 2024

miss-islington-app bot commented Jun 3, 2024

bedevere-app bot commented Jun 3, 2024

	/* -23:59 */
	PyObject *min = create_timezone_from_delta(-1, 60, 0, 1);
	DATETIME_ADD_MACRO(d, "min", min);

	/* +23:59 */
	PyObject max = create_timezone_from_delta(0, (23 60 + 59) * 60, 0, 0);
	DATETIME_ADD_MACRO(d, "max", max);

gh-117398: Add datetime Module State #119810

gh-117398: Add datetime Module State #119810

Conversation

ericsnowcurrently commented May 30, 2024 • edited Loading

ericsnowcurrently commented May 30, 2024 • edited Loading

ericsnowcurrently left a comment

Choose a reason for hiding this comment

neonene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neonene May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently commented May 31, 2024

neonene commented Jun 1, 2024

ericsnowcurrently commented Jun 1, 2024

ericsnowcurrently commented Jun 1, 2024

encukou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently commented Jun 3, 2024

ericsnowcurrently commented Jun 3, 2024

ericsnowcurrently commented Jun 3, 2024

miss-islington-app bot commented Jun 3, 2024

bedevere-app bot commented Jun 3, 2024

ericsnowcurrently commented May 30, 2024 •

edited

Loading

ericsnowcurrently commented May 30, 2024 •

edited

Loading

neonene May 31, 2024 •

edited

Loading