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.

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

Switch METH_VARGS to METH_O #679

Closed

Conversation

movermeyer
Copy link

What are you trying to accomplish?

While working on ciso8601, I discovered that things run a lot faster if you define your methods as METH_O (i.e., method with a single object argument) you can skip the overhead of PyArg_ParseTuple.

What approach did you choose and why?

Switched the instances of METH_VARGS to be METH_O instead.

The impact of these changes

Item Performance of parse_iso8601
master 1000000, 0.2697502020164393 => #270 nsec
This PR 1000000, 0.22211680599139072 => #222 nsec
v2.1.2 2000000, 0.3753614659945015 => #188 nsec

This PR shaves ~48 ns (17%) off the runtime of parse_iso8601.

Testing

Here's the script I used.

import timeit

UNITS = {"nsec": 1e-9, "usec": 1e-6, "msec": 1e-3, "sec": 1.0}
SCALES = sorted([(scale, unit) for unit, scale in UNITS.items()], reverse=True)

def format_duration(duration):
    # Based on cPython's `timeit` CLI formatting
    scale, unit = next(((scale, unit) for scale, unit in SCALES if duration >= scale), SCALES[-1])
    precision = 3
    return "%.*g %s" % (precision, duration / scale, unit)


timer = timeit.Timer(stmt="parse_iso8601('2014-01-09T21:48:00')", setup="from pendulum.parsing import parse_iso8601")
count, time_taken = timer.autorange()
timing = float(time_taken) / int(count)
print(f"{count}, {time_taken} => #{format_duration(timing)}")

What should reviewers focus on?

I didn't bother supporting Python 2.7 in these changes. It's fairly easy to do so, but I noticed that pendulum dropped support for Python < 3.7, so I didn't bother.

I'm not sure why v2.1.2 performed so well in this test. I didn't look into it. 🤷

Pull Request Check List

  • Added tests for changed code.
    • Interface didn't change.
  • Updated documentation for changed code.
    • Interface didn't change. No publicly facing changes. Just performance.

Much better performance
@Secrus
Copy link
Collaborator

Secrus commented Aug 5, 2023

Thanks for contributing, but our C extensions are being rewritten into Rust. Superseded by #721

@Secrus Secrus closed this Aug 5, 2023
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.

2 participants