gh-148085: datetime cache time module lookups#148088
gh-148085: datetime cache time module lookups#148088maurycy wants to merge 6 commits intopython:mainfrom
datetime cache time module lookups#148088Conversation
StanFromIreland
left a comment
There was a problem hiding this comment.
We don't have a precedent for lazy caching, I think it's always eager? I'm worried this might cause issues, historically datetime and caching hasn't worked out.
|
@StanFromIreland We do: https://github.com/python/cpython/blob/dea4083aa95/Modules/arraymodule.c#L2561 https://github.com/python/cpython/blob/dea4083aa95/Modules/_decimal/_decimal.c#L3699 I copied my approach from there. I'm OK with moving to I believe the reason why it didn't work is that it relied on a global variable, instead of per-interpreter module. I left a note in #148085 in the previous discussions feature section. |
| return NULL; | ||
| } | ||
| if (st->time_time == NULL) { | ||
| st->time_time = PyImport_ImportModuleAttrString("time", "time"); |
There was a problem hiding this comment.
Thinking more about this, I think we could just replace this with a call to PyTime_Time directly, I assume this predates the existence of the C-API.
|
I remember there were issues with datetime and the capsule API so be careful there. Likewise, if you do per-state caching, it may break existing hooks (removing the module from sys.modules will not make it reload I think) |
See gh-148085 for more details.
This takes a lazy approach, in the spirit of
lazy, instead of preloading ininit_state(). The trade-off is decreased readability.Please see Links to previous discussion of this feature section in the gh-148085. My understanding is that only @ericsnowcurrently's work in gh-119810 made this possible and clean.
Benchmark
For:
For: