-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
gh-88473: Implement fast path in date.today() for date types #130980
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Are the benchmarks realised on a PGO build or a debug build? or maybe PGO+LTO? or just release build? |
Both are just plain builds. python3.14 is from the morning but I doubt the few commits matter. |
Misc/NEWS.d/next/Library/2025-03-08-17-07-00.gh-issue-88473.qg23g8.rst
Outdated
Show resolved
Hide resolved
By plain builds, do you mean with or withou |
@pganssle can you take a look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my part, it looks good but I'll let a datetime expert have the final decision.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Friendly ping @pganssle Any comments on this? |
Friendly ping @pganssle :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an acceptable way to do it, but IIRC we already pushed some of this kind of dispatching logic in new_date_subclass_ex
, so I think you can replace the entire contents of date_today
with this:
static PyObject *
date_today(PyObject *cls, PyObject *Py_UNUSED(dummy))
{
/* Use C implementation to boost performance for date type */
struct tm tm;
time_t t;
time(&t);
if (_PyTime_localtime(t, &tm) != 0) {
return NULL;
}
return new_date_subclass_ex(tm.tm_year + 1900,
tm.tm_mon + 1,
tm.tm_mday,
(PyObject *)cls);
}
And you'll get the same speedup and as a bonus datetime.today()
will also get the same speedup.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
# Conflicts: # Modules/_datetimemodule.c
It will be incorrect though? Since |
Hah, oops, right, right. Should have run the test suite and not just the benchmark. OK, let's go with this as it is and if we want to speed up |
Other suggested implementations are not as backward compatible.
~5x faster for date types