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_FUNC PyLong_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)
        if min_val < 0:
            values += (-1, min_val)

to

        values = (0, 1, 512, 1234, max_val)
        if min_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_NSMALLPOSINTS           257
#define _PY_NSMALLNEGINTS           5

const Py_ssize_t PyLong_MASK_ssize_t = static_cast<Py_ssize_t>(PyLong_MASK);

void bench(Py_ssize_t val)
{
    const auto start{ std::chrono::steady_clock::now() };

    for (size_t i = 0; i < 10000000; ++i)
    {
        PyObject* obj = PyLong_FromSsize_t(val);
        Py_DECREF(obj);
    }

    const auto 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";
}

int main()
{
    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.