Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-71587: Isolate_datetime#102995

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedMar 24, 2023•
edited by bedevere-bot
Loading

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

erlend-aasland commentedMar 24, 2023•
edited
Loading

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 intestcapi:

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()?

erlend-aasland reacted with thumbs up emoji
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. Theget_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).

erlend-aasland reacted with eyes emoji
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner left review comments

@kumaraditya303kumaraditya303Awaiting requested review from kumaraditya303

@pgansslepganssleAwaiting requested review from pgansslepganssle will be requested when the pull request is marked ready for reviewpganssle is a code owner

@abalkinabalkinAwaiting requested review from abalkinabalkin will be requested when the pull request is marked ready for reviewabalkin is a code owner

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently will be requested when the pull request is marked ready for reviewericsnowcurrently is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@erlend-aasland@vstinner@bedevere-bot
[8]ページ先頭
©2009-2025 Movatter.jp