gh-129149: Add fast path for medium-size integers in PyLong_FromSsize_t()

[chris-eibl]

Add the macro PYLONG_FROM_INT and use it in PyLong_FromLong() and PyLong_FromLongLong().
There, this is just a binary compatible refactoring.

Use it in PyLong_FromSsize_t(), too, to get the fast path for medium-size integers.

• Issue: Missing fast path in PyLong_From*() functions for compact integers #129149

[skirpichev]

LGTM

Few stylistic nitpicks, feel free to ignore.

[chris-eibl]

I like your suggestions, brings us more in sync with PYLONG_FROM_UINT.
Just wanted to keep the diff small.

[skirpichev]

BTW, next time you can apply several suggestions in one shot (subscribers will get less notifications): it's possible add them in one batch (on tab "files changed").

[chris-eibl]

BTW, next time you can apply several suggestions in one shot (subscribers will get less notifications): it's possible add them in one batch (on tab "files changed").

Great, will do!

I've done some more syncs with PYLONG_FROM_UINT, if we want to keep the diff small, we can remove (all of) them.

[skirpichev]

@chris-eibl, please don't sync with the main branch, unless you want to fix file conflict or trigger a CI build.

[chris-eibl]

Just to reassure: this needs no further action from me atm - correct?

Most probably, another core reviewer is needed since it is still labeled as "awaiting core review"?

Do I have to request this? If yes, whom should I ask for a review?

[skirpichev]

CC @picnixz

[picnixz]

Out of curiosity, can we have some benchmarks?

[chris-eibl]

Out of curiosity, can we have some benchmarks?

pyperformance was neutral on it. I did think of some micro benchmarks, but I've found no good candidate without writing an external c application to directly benchmark the PyAPI_FUNCPyLong_AsSsize_t, yet.

Maybe something with ctypes would work out, too?

[picnixz]

Can you actually tests for checking the medium-size path? as well as perhaps checks for checking SSIZE_T_MIN (namely, tests to testcapi)

[chris-eibl]

Yeah, check_long_asint uses 1234, which is a medium-size int. I could add more.
As you suggest, best in there, so that we get more coverage for the other test_long_as*.

Especially one negative medium-size int would be fine, too?

Let's change

values= (0, 1, 1234, max_val)
ifmin_val<0:
values+= (-1, min_val)

to

values= (0, 1, 512, 1234, max_val)
ifmin_val<0:
values+= (-1, -512, -1234, min_val)

[chris-eibl]

The bots are failing. Seems not related to the PR.

Maybe updating with the main branch can fix it - shall I give it a try?

[picnixz]

yes, the CI config changed

[chris-eibl]

Yeah - that fixed it :)

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @picnixz for commit fda2844 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F129301%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[picnixz]

LGTM, conditioned to build bot success (one build bot is know to fail though)

[picnixz]

So the 2 build bots currently failing are know to fail so I think we can safely merge this one. Just asking Victor if he wants to add something else here.

[chris-eibl]

Here are some benchmarks:

• on my Windows 10 PC (i5-4570 CPU)
• Microsoft Visual Studio 2022 17.13.0 Preview 5.0
• release build (non-PGO)
• about 25% faster

before:

some small ints
-5: 21.26 ms
0: 20.83 ms
256: 20.90 ms
some medium ints
-1073741823: 123.28 ms
1000: 124.79 ms
1073741822: 122.32 ms
some bigger ints
-1073741824: 196.28 ms
1073741824: 195.87 ms

after:

some small ints
-5: 20.90 ms
0: 20.45 ms
256: 20.50 ms
some medium ints
-1073741823: 100.39 ms
1000: 98.54 ms
1073741822: 98.61 ms
some bigger ints
-1073741824: 198.79 ms
1073741824: 198.45 ms

Details

#include<chrono>
#include<iomanip>
#include<iostream>
#include"Python.h"
#define_PY_NSMALLPOSINTS257
#define_PY_NSMALLNEGINTS5const Py_ssize_t PyLong_MASK_ssize_t = static_cast<Py_ssize_t>(PyLong_MASK);
voidbench(Py_ssize_t val)
{
constauto start{ std::chrono::steady_clock::now() };
for (size_t i = 0; i < 10000000; ++i)
{
PyObject* obj = PyLong_FromSsize_t(val);
Py_DECREF(obj);
}
constauto finish{ std::chrono::steady_clock::now() };
const std::chrono::duration<double> elapsed_seconds{ finish - start };
std::cout << std::setw(12) << val << ": " << std::setw(8) << elapsed_seconds.count() * 1000 << " ms\n";
}
intmain()
{
Py_Initialize();
std::cout << "some small ints\n";
std::cout << std::setprecision(2) << std::fixed;
bench(-_PY_NSMALLNEGINTS);
bench(0);
bench(_PY_NSMALLPOSINTS - 1);
std::cout << "some medium ints\n";
bench(0 - PyLong_MASK_ssize_t);
bench(1000);
bench(PyLong_MASK_ssize_t - 1);
std::cout << "some bigger ints\n";
bench(0 - PyLong_MASK_ssize_t - 1);
bench(PyLong_MASK_ssize_t + 1);
Py_Finalize();
}

[picnixz]

The benchmarks are promising. We are gaining like 10-20% so it's non-negligible.

[vstinner]

LGTM. Interesting optimization.

cc @serhiy-storchaka

[chris-eibl]

@vstinner: shall PyLong_FromInt32(), PyLong_FromUInt32() and PyLong_FromUInt64() use these macros in a follow up PR, too? AFAIR you've introduced those methods.

[vstinner]

@vstinner: shall PyLong_FromInt32(), PyLong_FromUInt32() and PyLong_FromUInt64() use these macros in a follow up PR, too? AFAIR you've introduced those methods.

Let's wait until this change PR is merged.

[picnixz]

I'll merge this by the end of the week (hence my assignment). @chris-eibl can you update the branch just to run the latest CI? TiA (I'll do it tomorrow otherwise)

[picnixz]

Thank you!

[picnixz]

shall PyLong_FromInt32(), PyLong_FromUInt32() and PyLong_FromUInt64() use these macros in a follow up PR, too? AFAIR you've introduced those methods.

You can reuse the same issue for this follow-up PR btw.