gh-127936: convert marshal module to use import/export API for ints (PEP 757) by skirpichev · Pull Request #128530 · python/cpython · GitHub

Issue: Remove private _PyLong_New() function #127936

Benchmark	ref-dumps	patch-dumps
dumps 1<<38	628 ns	640 ns: 1.02x slower
dumps 1<<300	635 ns	696 ns: 1.10x slower
dumps 1<<3000	2.02 us	2.28 us: 1.13x slower
Geometric mean	(ref)	1.06x slower

Benchmark hidden because not significant (1): dumps 1<<7

Benchmark	ref-loads	patch-loads
loads 1<<7	303 ns	311 ns: 1.03x slower
loads 1<<38	334 ns	388 ns: 1.16x slower
loads 1<<300	516 ns	605 ns: 1.17x slower
loads 1<<3000	2.08 us	2.56 us: 1.23x slower
Geometric mean	(ref)	1.15x slower

scripts

# bench-dumps.py

from marshal import dumps
import pyperf

values = ['1<<7', '1<<38', '1<<300', '1<<3000']
runner = pyperf.Runner()
for v in values:
    i = eval(v)
    bn = 'dumps '+v
    runner.bench_func(bn, dumps, i)

# bench-loads.py

from marshal import loads, dumps
import pyperf

values = ['1<<7', '1<<38', '1<<300', '1<<3000']
runner = pyperf.Runner()
for v in values:
    d = dumps(eval(v))
    bn = 'loads '+v
    runner.bench_func(bn, loads, d)

…ints (PEP 757)

CC @vstinner

I suspect that major slowdown for marshal.loads() benchmarks is due to normalization&singletonization in the PyLongWriter_Finish(). In case of marshal.dumps() I suspect things were better without the value field.

Co-authored-by: Victor Stinner <vstinner@python.org>

LGTM. I'm (just) a bit unfortunate that the using PEP 757 makes the code slower.

@serhiy-storchaka @picnixz: Would you mind to review this change?

cc @picnixz @serhiy-storchaka

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

A couple of assertions to add and we're good I think. The most important assertion is assert(layout->bits_per_digit >= PyLong_MARSHAL_SHIFT); because marshal_ratio is 0 otherwise.

I would still appreciate an assert on any T that does digits[T - 1] for those who only read the macros (or for reviewers) even though you're asserting that at the caller site. The assertions cost nothing and it's semantically better to have assertions verified once you've called the function before calling it (for debugging it's easier to put them before the call so that you know which caller was the bad caller, however I think that assertions should be logically placed in the callee; thus I would put them at both places).

Now, I won't block on this nitpick since it's a matter of taste.

I would still appreciate an assert on any T that does digits[T - 1] for those who only read the macros (or for reviewers) even though you're asserting that at the caller site.

Of course, assert's costs nothing, but I doubt we need this redundancy. Macros code not too big to see obvious assumptions on n/size arguments.

So, I'll add suggestions and postpone decision to other reviewers.

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

@skirpichev: Can you just add assert(size >= 1);? I don't want to apply your suggestion because of a missing space: size>= 1.

Of course, assert's costs nothing, but I doubt we need this redundancy. Macros code not too big to see obvious assumptions on n/size arguments.

While the code is obvious to you, assertions can help reviewers, who are not used to the code, to understand it. It's not only to check for correctness at runtime.

Can you just add assert(size >= 1);?

Done. Feel free just to edit my pr/suggestions in similar cases.

While the code is obvious to you, assertions can help reviewers

Well, just the next "for" line, IMO, revival code assumptions for most code readers.

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

Merged. Congrats @skirpichev, this one was tricky :-)

bedevere-app Bot mentioned this pull request Jan 6, 2025

Remove private _PyLong_New() function #127936

Closed

skirpichev added the skip news label Jan 6, 2025

pythongh-127936 : convert marshal module to use import/export API for …

4bd6b0c

…ints (PEP 757)

skirpichev force-pushed the port-marshal-to-pep757/127936 branch from 1ab0c30 to 4bd6b0c Compare January 7, 2025 05:06

skirpichev marked this pull request as ready for review January 7, 2025 05:34

bedevere-app Bot added the awaiting review label Jan 7, 2025

vstinner reviewed Jan 7, 2025

View reviewed changes

Comment thread Python/marshal.c

Comment thread Python/marshal.c

Comment thread Python/marshal.c

Comment thread Python/marshal.c Outdated

Comment thread Python/marshal.c Outdated

Comment thread Python/marshal.c

skirpichev and others added 4 commits January 7, 2025 13:25

Apply suggestions from code review

44c7c6a

Co-authored-by: Victor Stinner <vstinner@python.org>

Merge branch 'master' into port-marshal-to-pep757/127936

e960c17

address review: local decl

95d89a7

Merge branch 'main' into port-marshal-to-pep757/127936

77be1bd

skirpichev requested a review from vstinner January 8, 2025 07:53

vstinner approved these changes Jan 8, 2025

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels Jan 8, 2025

skirpichev requested a review from serhiy-storchaka January 8, 2025 13:11

skirpichev commented Jan 9, 2025

View reviewed changes

Comment thread Python/marshal.c

skirpichev commented Jan 9, 2025

View reviewed changes

Comment thread Python/marshal.c

picnixz reviewed Jan 14, 2025

View reviewed changes

skirpichev commented Jan 14, 2025

View reviewed changes

Comment thread Python/marshal.c Outdated

Comment thread Python/marshal.c

skirpichev and others added 2 commits January 14, 2025 17:31

Apply suggestions from code review

1e0db95

Apply suggestions from code review

d66af47

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

picnixz reviewed Jan 14, 2025

View reviewed changes

Comment thread Python/marshal.c

Comment thread Python/marshal.c Outdated

skirpichev and others added 3 commits January 14, 2025 17:43

Update Python/marshal.c

f152274

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Merge branch 'master' into port-marshal-to-pep757/127936

fb505db

address review: bitsize (naming)

035c772

skirpichev requested a review from picnixz January 14, 2025 15:23

picnixz reviewed Jan 14, 2025

View reviewed changes

Comment thread Python/marshal.c

Comment thread Python/marshal.c

Comment thread Python/marshal.c

skirpichev commented Jan 15, 2025

View reviewed changes

Comment thread Python/marshal.c

Comment thread Python/marshal.c

Comment thread Python/marshal.c

Apply suggestions from code review

9d95c35

skirpichev requested a review from picnixz January 15, 2025 03:52

picnixz approved these changes Jan 15, 2025

View reviewed changes

skirpichev commented Jan 15, 2025

View reviewed changes

Comment thread Python/marshal.c

Comment thread Python/marshal.c

Update Python/marshal.c

47eaacf

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

Update Python/marshal.c

9353013

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

vstinner enabled auto-merge (squash) January 23, 2025 02:07

Merge branch 'main' into port-marshal-to-pep757/127936

82cb784

vstinner merged commit d7d066c into python:main Jan 23, 2025

bedevere-app Bot removed the awaiting merge label Jan 23, 2025

skirpichev deleted the port-marshal-to-pep757/127936 branch January 23, 2025 02:57

gh-127936: convert marshal module to use import/export API for ints (PEP 757) by skirpichev · Pull Request #128530 · python/cpython · GitHub

Repository navigation

gh-127936: convert marshal module to use import/export API for ints (PEP 757) - #128530

gh-127936: convert marshal module to use import/export API for ints (PEP 757)#128530vstinner merged 14 commits intopython:mainpython/cpython:mainfrom skirpichev:port-marshal-to-pep757/127936skirpichev/cpython:port-marshal-to-pep757/127936Copy head branch name to clipboard

Conversation

skirpichev commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skirpichev commented Jan 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jan 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

skirpichev commented Jan 15, 2025

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jan 23, 2025

Uh oh!

vstinner commented Jan 23, 2025

Uh oh!

skirpichev commented Jan 23, 2025

Uh oh!

vstinner commented Jan 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gh-127936: convert marshal module to use import/export API for ints (PEP 757)#128530
vstinner merged 14 commits into
python:mainfrom
skirpichev:port-marshal-to-pep757/127936

skirpichev commented Jan 6, 2025 •
edited
Loading