8000 Port zoneinfo module to use module state · Issue #99138 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

Port zoneinfo module to use module state #99138

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

Jump to bottom
Closed
sobolevn opened this issue Nov 5, 2022 · 4 comments
Closed

Port zoneinfo module to use module state #99138

sobolevn opened this issue Nov 5, 2022 · 4 comments
Assignees
@sobolevn
Labels
topic-subinterpreters type-feature A feature request or enhancement

Comments

@sobolevn
Copy link
Member
sobolevn commented Nov 5, 2022 •
edited by bedevere-bot
Loading

Feature or enhancement

Following https://github.com/ericsnowcurrently/multi-core-python/wiki/0-The-Plan we need to convert _zoneinfo to use module state.

Pitch

Right now there are several global objects:

cpython/Modules/_zoneinfo.c

Lines 23 to 25 in 47ab848

static PyObject *io_open = NULL;
static PyObject *_tzpath_find_tzfile = NULL;
static PyObject *_common_mod = NULL;

and

cpython/Modules/_zoneinfo.c

Lines 96 to 101 in 47ab848

static PyObject *TIMEDELTA_CACHE = NULL;
static PyObject *ZONEINFO_WEAK_CACHE = NULL;
static StrongCacheNode *ZONEINFO_STRONG_CACHE = NULL;
static size_t ZONEINFO_STRONG_CACHE_MAX_SIZE = 8;
static _ttinfo NO_TTINFO = {NULL, NULL, NULL, 0};

And one static type definition:

cpython/Modules/_zoneinfo.c

Line 93 in 47ab848

static PyTypeObject PyZoneInfo_ZoneInfoType;

If this is indeed planned to be fixed, I would love to work on it.
@sobolevn sobolevn added type-feature A feature request or enhancement topic-subinterpreters labels Nov 5, 2022
@sobolevn sobolevn self-assigned this Nov 5, 2022
@corona10
Copy link
Member
corona10 commented Nov 6, 2022 •
edited
Loading

I would like to listen to the opinion of the original author @pganssle

@pganssle
Copy link
Member
pganssle commented Nov 6, 2022 •
edited
Loading

I think I originally planned to do this but it made the implementation a bit more complicated and maybe all the pieces to do it weren't in place / stable yet, and after talking to Eric it seemed like it wasn't time sensitive back then.

I don't remember any major show stoppers, just, "This is hard enough, I should spend my effort points on other stuff that matters more." Which isn't to say there aren't show stoppers, just that I don't remember any.

I think the big thing to be aware of when doing this is that the weak cache is not for performance, it is guaranteeing specific semantics, detailed in PEP 615. So hopefully it is possible to do this while maintaining the relevant guarantees.

@erlend-aasland
Copy link
Contributor
erlend-aasland commented Nov 6, 2022 •
edited
Loading

FWIW, I have a two year old branch (now rebased onto main) that converts PyZoneInfo_ZoneInfoType to a heap type and establishes a global state struct:

https://github.com/erlend-aasland/cpython/pull/new/isolate-zoneinfo

@bedevere-bot bedevere-bot mentioned this issue Nov 7, 2022
Merged
@erlend-aasland
Copy link
Contributor

I had some time free tonight, so I tried to complete my old branch and created a draft PR (gh-99218).

erlend-aasland added a commit that referenced this issue Feb 15, 2023
@erlend-aasland
gh-99138: Isolate _zoneinfo (#99218)
c1ce0d1 
* Convert zone info type to heap type and add it to module state
* Add global variables to module state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sobolevn sobolevn

Labels
topic-subinterpreters type-feature A feature request or enhancement
Projects
Subinterpreters
Status: Done
Milestone
No milestone
Development

No branches or pull requests
4 participants
@pganssle @sobolevn @corona10 @erlend-aasland
0