Skip to content
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

gh-111482: Use Argument Clinic for clock_gettime() #111641

Merged
merged 3 commits into from
Nov 2, 2023
Merged

gh-111482: Use Argument Clinic for clock_gettime() #111641

merged 3 commits into from
Nov 2, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 2, 2023 •
edited by bedevere-app bot
Loading

Use Argument Clinic for time.clock_gettime() and
time.clock_gettime_ns() functions.

Benchmark on time.clock_gettime_ns():

import time
import pyperf
runner = pyperf.Runner()
runner.timeit(
    'clock_gettime_ns(CLOCK_MONOTONIC_COARSE)',
    setup='import time; clock_gettime_ns=time.clock_gettime_ns; CLOCK_MONOTONIC_COARSE=6',
    stmt='clock_gettime_ns(CLOCK_MONOTONIC_COARSE)')

Result on Linux with CPU isolation:

Mean +- std dev: [ref] 134 ns +- 1 ns -> [change] 55.7 ns +- 1.4 ns: 2.41x faster

@vstinner
pythongh-111482: Use Argument Clinic for clock_gettime()
09cb5b3 
Use Argument Clinic for time.clock_gettime() and
time.clock_gettime_ns() functions.

Benchmark on time.clock_gettime_ns():

    import time
    import pyperf
    runner = pyperf.Runner()
    runner.timeit(
        'clock_gettime_ns(CLOCK_MONOTONIC_COARSE)',
        setup='import time; clock_gettime_ns=time.clock_gettime_ns; CLOCK_MONOTONIC_COARSE=6',
        stmt='clock_gettime_ns(CLOCK_MONOTONIC_COARSE)')

Result on Linux with CPU isolation:

Mean +- std dev: [ref] 134 ns +- 1 ns -> [change] 55.7 ns +- 1.4 ns: 2.41x faster
@bedevere-app bedevere-app bot mentioned this pull request Nov 2, 2023
Closed
@vstinner
Copy link
Member Author

vstinner commented Nov 2, 2023

cc @corona10

@corona10
Copy link
Member

corona10 commented Nov 2, 2023

@vstinner
Please run the following command :)

make regen-all

corona10
corona10 approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Overall LGTM

@serhiy-storchaka serhiy-storchaka self-requested a review November 2, 2023 12:13
serhiy-storchaka
serhiy-storchaka reviewed Nov 2, 2023
View reviewed changes
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.

Wow, it is impressive! I did not know there are still such low hanging fruits.

This PR consists of two parts.

  1. clock_gettime() supported 64-bit clockid_t on AIX. This PR makes clock_gettime_ns() also supporting it. It looks rather like a bugfix, and I think it should be backported. clock_settime() and clock_settime_ns() also should support it.
  2. Using Argument Clinic instead of PyArg_ParseTuple().

Modules/_testinternalcapi.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

vstinner commented Nov 2, 2023

make regen-all

Done. I did it, but then I made further changes. Apparently, I had to run it again.

@vstinner
Copy link
Member Author

vstinner commented Nov 2, 2023

Wow, it is impressive! I did not know there are still such low hanging fruits.

Honestly, I'm disappointed that PyArg_ParseTuple() is so slow :-( I suppose that creating a temporary tuple to pass positional arguments also has a cost. But yeah, it's a nice optimization.

clock_gettime() supported 64-bit clockid_t on AIX. This PR makes clock_gettime_ns() also supporting it. It looks rather like a bugfix, and I think it should be backported. clock_settime() and clock_settime_ns() also should support it.

We don't support AIX. I just tried to not regress. I don't plan to backport Argument Clinic code nor enhance time.clock_gettime_ns() on AIX in 3.12.

@vstinner vstinner merged commit 4fe22c7 into python:main Nov 2, 2023
25 checks passed
@vstinner vstinner deleted the clinic_clock_gettime branch November 2, 2023 13:29
@vstinner
Copy link
Member Author

vstinner commented Nov 2, 2023

Thanks for reviews.

@serhiy-storchaka: If you are motivated to enhance AIX support in 3.12, please go ahead ;-) The two AIX buildbots are failing for many months... I'm not even sure why we still have AIX buildbots.

FullteaR pushed a commit to FullteaR/cpython that referenced this pull request Nov 3, 2023
@vstinner @FullteaR
pythongh-111482: Use Argument Clinic for clock_gettime() (python#111641)
f1bae1d 
Use Argument Clinic for time.clock_gettime() and
time.clock_gettime_ns() functions.

Benchmark on time.clock_gettime_ns():

    import time
    import pyperf
    runner = pyperf.Runner()
    runner.timeit(
        'clock_gettime_ns(CLOCK_MONOTONIC_COARSE)',
        setup='import time; clock_gettime_ns=time.clock_gettime_ns; CLOCK_MONOTONIC_COARSE=6',
        stmt='clock_gettime_ns(CLOCK_MONOTONIC_COARSE)')

Result on Linux with CPU isolation:

Mean +- std dev: [ref] 134 ns +- 1 ns -> [change] 55.7 ns +- 1.4 ns: 2.41x faster
@ayappanec ayappanec mentioned this pull request Nov 3, 2023
Closed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@vstinner @aisk
pythongh-111482: Use Argument Clinic for clock_gettime() (python#111641)
43a07f3 
Use Argument Clinic for time.clock_gettime() and
time.clock_gettime_ns() functions.

Benchmark on time.clock_gettime_ns():

    import time
    import pyperf
    runner = pyperf.Runner()
    runner.timeit(
        'clock_gettime_ns(CLOCK_MONOTONIC_COARSE)',
        setup='import time; clock_gettime_ns=time.clock_gettime_ns; CLOCK_MONOTONIC_COARSE=6',
        stmt='clock_gettime_ns(CLOCK_MONOTONIC_COARSE)')

Result on Linux with CPU isolation:

Mean +- std dev: [ref] 134 ns +- 1 ns -> [change] 55.7 ns +- 1.4 ns: 2.41x faster
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@vstinner @Glyphack
pythongh-111482: Use Argument Clinic for clock_gettime() (python#111641)
0b7de23 
Use Argument Clinic for time.clock_gettime() and
time.clock_gettime_ns() functions.

Benchmark on time.clock_gettime_ns():

    import time
    import pyperf
    runner = pyperf.Runner()
    runner.timeit(
        'clock_gettime_ns(CLOCK_MONOTONIC_COARSE)',
        setup='import time; clock_gettime_ns=time.clock_gettime_ns; CLOCK_MONOTONIC_COARSE=6',
        stmt='clock_gettime_ns(CLOCK_MONOTONIC_COARSE)')

Result on Linux with CPU isolation:

Mean +- std dev: [ref] 134 ns +- 1 ns -> [change] 55.7 ns +- 1.4 ns: 2.41x faster
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@corona10 corona10 corona10 approved these changes

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner

@abalkin abalkin Awaiting requested review from abalkin abalkin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vstinner @corona10 @serhiy-storchaka