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

[skirpichev]

• 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)

[skirpichev]

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.

[vstinner]

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?

[vstinner]

cc @picnixz @serhiy-storchaka

[picnixz]

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.

[picnixz]

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.

[skirpichev]

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.

[vstinner]

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

[vstinner]

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.

[skirpichev]

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.

[vstinner]

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