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

gh-102471: convert decimal module to use PyLong_Export API (PEP 757) #128267

Merged
merged 7 commits into from
Jan 6, 2025

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Dec 26, 2024

For reviewers: in the history included version, where PyLong_Export always set digits.

Benchmark ref patch no-value
Decimal(1<<7) 735 ns not significant 721 ns: 1.02x faster
Decimal(1<<38) 809 ns 738 ns: 1.10x faster 846 ns: 1.05x slower
Decimal(1<<300) 1.93 us 2.05 us: 1.06x slower 1.99 us: 1.03x slower
Geometric mean (ref) 1.01x faster 1.01x slower

Benchmark hidden because not significant (1): Decimal(1<<3000)

benchmark code
# export_bench.py
import pyperf
from decimal import Decimal as D

runner = pyperf.Runner()
i1, i2, i3, i4 = 1 << 7, 1 << 38, 1 << 300, 1 << 3000
runner.bench_func('Decimal(1<<7)', D, i1)
runner.bench_func('Decimal(1<<38)', D, i2)
runner.bench_func('Decimal(1<<300)', D, i3)
runner.bench_func('Decimal(1<<3000)', D, i4)

Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
@skirpichev
Copy link
Member Author

@vstinner, @serhiy-storchaka, please review.

I removed const's, added assert's, also handling value case now more simple. It seems, difference too small to measure on my system:

Benchmark ref patch only-qset_i64
int(Decimal(1<<7)) 620 ns 634 ns: 1.02x slower 634 ns: 1.02x slower
int(Decimal(1<<38)) 697 ns 728 ns: 1.04x slower 719 ns: 1.03x slower
int(Decimal(1<<300)) 2.02 us 2.04 us: 1.01x slower 2.04 us: 1.01x slower
Geometric mean (ref) 1.02x slower 1.02x slower

Benchmark hidden because not significant (1): int(Decimal(1<<3000))

(*) only-qset_i64 - current version.

@serhiy-storchaka, if you prefer runtime checks over asserts - I'll cache layout parameters in the decimal module state (though, seems odd for me).

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. Thanks.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I'm a little bit sad that the change makes the code a little bit slower, but I guess that's the price of abstraction.

@vstinner vstinner merged commit 879d287 into python:main Jan 6, 2025
43 checks passed
@vstinner
Copy link
Member

vstinner commented Jan 6, 2025

Merged, thanks.

@skirpichev skirpichev deleted the decimal-use-PyLong_Export/127937 branch January 6, 2025 10:54
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants