Skip to content

gh-112919: Speed-up datetime, date and time.replace() #112921

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

Merged

Conversation

eltoder
Copy link
Contributor

@eltoder eltoder commented Dec 10, 2023

Use argument clinic and call new_* functions directly. This speeds up these functions 6x to 7.5x when calling with keyword arguments.

Before:

$ python -m timeit -s "from datetime import datetime; dt = datetime.now()" "dt.replace(microsecond=0)"
500000 loops, best of 5: 501 nsec per loop

$ python -m timeit -s "from datetime import date; d = date.today()" "d.replace(day=1)"
1000000 loops, best of 5: 273 nsec per loop

$ python -m timeit -s "from datetime import time; t = time()" "t.replace(microsecond=0)"
1000000 loops, best of 5: 338 nsec per loop

After:

$ python -m timeit -s "from datetime import datetime; dt = datetime.now()" "dt.replace(microsecond=0)"
5000000 loops, best of 5: 65.9 nsec per loop

$ python -m timeit -s "from datetime import date; d = date.today()" "d.replace(day=1)"
5000000 loops, best of 5: 45.6 nsec per loop

$ python -m timeit -s "from datetime import time; t = time()" "t.replace(microsecond=0)"
5000000 loops, best of 5: 51 nsec per loop

Use argument clinic and call new_* functions directly. This speeds up
these functions 6x to 7.5x when calling with keyword arguments.

Before:
$ python -m timeit -s "from datetime import datetime; dt = datetime.now()" "dt.replace(microsecond=0)"
500000 loops, best of 5: 501 nsec per loop

$ python -m timeit -s "from datetime import date; d = date.today()" "d.replace(day=1)"
1000000 loops, best of 5: 273 nsec per loop

$ python -m timeit -s "from datetime import time; t = time()" "t.replace(microsecond=0)"
1000000 loops, best of 5: 338 nsec per loop

After:
$ python -m timeit -s "from datetime import datetime; dt = datetime.now()" "dt.replace(microsecond=0)"
5000000 loops, best of 5: 65.9 nsec per loop

$ python -m timeit -s "from datetime import date; d = date.today()" "d.replace(day=1)"
5000000 loops, best of 5: 45.6 nsec per loop

$ python -m timeit -s "from datetime import time; t = time()" "t.replace(microsecond=0)"
5000000 loops, best of 5: 51 nsec per loop
@eltoder eltoder force-pushed the feature/speedup-datetime-replace branch 2 times, most recently from ca3b11f to 53a9b6f Compare December 10, 2023 13:13
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

In general LGTM, but please generate also __replace__ methods.

Comment on lines +3598 to +3600
DATETIME_DATE_REPLACE_METHODDEF

{"__replace__", _PyCFunction_CAST(date_replace), METH_VARARGS | METH_KEYWORDS},
{"__replace__", _PyCFunction_CAST(datetime_date_replace), METH_FASTCALL | METH_KEYWORDS},
Copy link
Member

Choose a reason for hiding this comment

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

There is an issue here. DATETIME_DATE_REPLACE_METHODDEF can be changed with different options of Argument Clinic (for example when we experiment with the limited C API). The generated datetime_date_replace can no longer be compatible with manually written entry for __replace__.

The simplest way to fix this is to add separate definition for __replace__:

/*[clinic input]
datetime.date.__replace__ = datetime.date.replace
[clinic start generated code]*/

static PyObject *
datetime_date___replace___impl(PyDateTime_Date *self, int year, int month,
                               int day)
/*[clinic end generated code: output=bfb97f926e24f201 input=1b2cac755dfa9b40]*/
{
    return new_date_ex(year, month, day, Py_TYPE(self));
}

Copy link
Contributor Author

@eltoder eltoder Dec 17, 2023

Choose a reason for hiding this comment

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

If I understand correctly, this means that the bodies of all replace() functions need to be duplicated. This means that all future changes to that code will need to be done twice, which is a bad thing. (The functions may seem trivial, but I'm already working on a further change to address #89039.) This will also generate the argument parsing code twice, which will bloat executable size. A better approach would be to add some syntax to argument clinic to generate the second METHODDEF.

Also, note that I copied this approach from code.__replace__, so if this is not right, code.__replace__ needs to be changed as well.

@eltoder
Copy link
Contributor Author

eltoder commented Dec 17, 2023

@serhiy-storchaka thank you for the review

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Well, let merge it as is, and make the code more independent from Argument Clinic changes in other issue. Otherwise we risk to forget about this PR.

@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) January 30, 2024 15:05
@serhiy-storchaka serhiy-storchaka merged commit 1f515e8 into python:main Jan 30, 2024
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…-112921)

Use argument clinic and call new_* functions directly. This speeds up
these functions 6x to 7.5x when calling with keyword arguments.
@eltoder eltoder deleted the feature/speedup-datetime-replace branch February 12, 2024 13:23
@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Feb 12, 2024
@miss-islington-app
Copy link

Thanks @eltoder for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @eltoder for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @eltoder and @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 1f515e8a109204f7399d85b7fd806135166422d9 3.11

@miss-islington-app
Copy link

Sorry, @eltoder and @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 1f515e8a109204f7399d85b7fd806135166422d9 3.12

eltoder added a commit to eltoder/cpython that referenced this pull request Feb 12, 2024
…-112921)

Use argument clinic and call new_* functions directly. This speeds up
these functions 6x to 7.5x when calling with keyword arguments.

(cherry picked from commit 1f515e8)
eltoder added a commit to eltoder/cpython that referenced this pull request Feb 12, 2024
…ythonGH-112921)

Use argument clinic and call new_* functions directly. This speeds up
these functions 6x to 7.5x when calling with keyword arguments.
(cherry picked from commit 1f515e8)

Co-authored-by: Eugene Toder <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Feb 12, 2024

GH-115344 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Feb 12, 2024
@erlend-aasland
Copy link
Contributor

Thanks, nice change!

@serhiy-storchaka serhiy-storchaka removed their assignment Sep 29, 2024
@serhiy-storchaka serhiy-storchaka removed the needs backport to 3.11 only security fixes label Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants