◐ Shell
reader mode source ↗
Skip to content

gh-71587: Establish global state in _datetime#110475

Merged
erlend-aasland merged 21 commits into
python:mainfrom
erlend-aasland:isolate-datetime/part1-establish-global-state
Oct 12, 2023
Merged

gh-71587: Establish global state in _datetime#110475
erlend-aasland merged 21 commits into
python:mainfrom
erlend-aasland:isolate-datetime/part1-establish-global-state

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Oct 6, 2023

Copy link
Copy Markdown
Contributor
  • Establish global state
  • Put singletons in global state

@erlend-aasland erlend-aasland changed the title gh-78469: Establish global state in _datetime Oct 6, 2023

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Maybe with shorter names, you can just write get_datetime_state()->utc directly (for example).

@erlend-aasland

Copy link
Copy Markdown
Contributor Author

PTAL, @vstinner

19 hidden items Load more…

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

If PyInit__datetime() fails, I suppose that the next call may override global state without clearing the previous strong refererence: reference leak. I don't think that we should bother since this state is going to become more dynamic and set differently, no?

@erlend-aasland

erlend-aasland commented Oct 11, 2023

Copy link
Copy Markdown
Contributor Author

If PyInit__datetime() fails, I suppose that the next call may override global state without clearing the previous strong refererence: reference leak. I don't think that we should bother since this state is going to become more dynamic and set differently, no?

Well, it is easy to already now add the machinery needed to clear the state (future m_clear function). It's easy to add such a function now, and call it in the PyInit__datetime() error path. It will be an improvement over status quo where global state is overwritten anyway.

Note that parts of the state actually is cleared if the module exec function fails.

@erlend-aasland

Copy link
Copy Markdown
Contributor Author

Perhaps we should start with a PR that cleans up _datetime init code?

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM, thanks.

@erlend-aasland

Copy link
Copy Markdown
Contributor Author

Thanks for the review, Victor!

@erlend-aasland erlend-aasland merged commit ec5622d into python:main Oct 12, 2023
@erlend-aasland erlend-aasland deleted the isolate-datetime/part1-establish-global-state branch October 12, 2023 08:28
@erlend-aasland

Copy link
Copy Markdown
Contributor Author

There are lots of paths we can take for the follow-up PRs.

Alt 1, continue adding to static state:

  • Convert static types to heap types; add them to static state
  • Convert static state to module state (will break encapsulated C API)
  • Deprecate encapsulated C API and introduce a new one based on module state

Alt 2, try to save the encapsulated C API by moving static state to the interpreter

  • Move existing static state to the interpreter
  • Convert static types to heap types; add them to the interpreter owned state struct

What do you think, Victor?

@vstinner

Copy link
Copy Markdown
Member

Convert static types to heap types; add them to static state

Sounds like a good step.

Move existing static state to the interpreter

I would prefer to consider that after converting types to heap types.

IMO the C API like PyDateTime_Check() will prevent to make this state per module. Instead, you can consider splitting this state in two states:

  • One state per interpreter: anything involing types and instances of these types
  • One state per module instance: constants like "microseconds per second"

You simply cannot have more than one instance of the same type beause of PyDateTime_Check().

PyDateTime_Check() will be per-interpreter. I had the exact same problem with PyAST_Check():

int PyAST_Check(PyObject* obj)
{
    struct ast_state *state = get_ast_state();
    if (state == NULL) {
        return -1;
    }
    return PyObject_IsInstance(obj, state->AST_type);
}

@vstinner

Copy link
Copy Markdown
Member
    if (state == NULL) {
        return -1;
    }

By the way, the error case could be avoided by running the initialization at Python startup.

@jackyk02

Copy link
Copy Markdown

Hi Erlend, would it be possible for you to support this for Python 3.12?

@erlend-aasland

Copy link
Copy Markdown
Contributor Author

Hi Erlend, would it be possible for you to support this for Python 3.12?

No; that would be a breaking change.

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
* Use explicit initialiser for m_base
* Add module state stub; establish global state on stack
* Put conversion factors in state struct
* Move PyDateTime_TimeZone_UTC to state
* Move PyDateTime_Epoch to state struct
* Fix ref leaks in and clean up initialisation
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