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-127937: convert decimal module to use import/export API for ints (PEP 757) #127925

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Dec 13, 2024

For export (int instance → Decimal)

Benchmark ref patch
Decimal(1<<7) 672 ns 708 ns: 1.05x slower
Decimal(1<<38) 764 ns 713 ns: 1.07x faster
Decimal(1<<300) 1.88 us 1.94 us: 1.03x slower
Decimal(1<<3000) 90.1 us 90.2 us: 1.00x slower
Geometric mean (ref) 1.00x slower

For import (Decimal instance → int)

Benchmark ref patch
int(Decimal(1<<7)) 609 ns 517 ns: 1.18x faster
int(Decimal(1<<38)) 712 ns 502 ns: 1.42x faster
int(Decimal(1<<300)) 2.04 us 1.97 us: 1.04x faster
int(Decimal(1<<3000)) 116 us 115 us: 1.00x faster
Geometric mean (ref) 1.15x faster
>>> sys.int_info[:2]
(30, 4)
benchmarks 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)
# import_bench.py
import pyperf
from decimal import Decimal as D

runner = pyperf.Runner()
d1, d2, d3, d4 = D(1 << 7), D(1 << 38), D(1 << 300), D(1 << 3000)
runner.bench_func('int(Decimal(1<<7))', int, d1)
runner.bench_func('int(Decimal(1<<38))', int, d2)
runner.bench_func('int(Decimal(1<<300))', int, d3)
runner.bench_func('int(Decimal(1<<3000))', int, d4)

@picnixz
Copy link
Contributor

picnixz commented Dec 13, 2024

hide _PyLong_FromDigits()? it's not used outside of the longobject.c anymore

Let's not hide this. Maybe someone is using it (it was removed then restored IIRC).

news

Not needed I think, unless you want to indicate the performance gain (it's always nice to know that something is faster). I did report the improvements of fnmatch.translate, so I think you can report those improvements as well.

n = (mpd_sizeinbase(x, 2) + bpd - 1) / bpd;
PyLongWriter *writer = PyLongWriter_Create(mpd_isnegative(x), n,
(void**)&ob_digit);
/* mpd_sizeinbase can overestimate size by 1 digit, set it to zero. */
Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, this looks as a bug in the mpdecimal. C.f. the GNU GMP, the mpz_sizeinbase docs says: "If base is a power of 2, the result is always exact".

@skirpichev
Copy link
Member Author

Let's not hide this. Maybe someone is using it (it was removed then restored IIRC).

I've updated the pr descriptions with my research. So far, I've found just one use case.

At least, I think we should deprecate (not soft) this. This apparently affects not so much projects and there is now a public alternative. @picnixz, what do you think?

@skirpichev skirpichev marked this pull request as ready for review December 14, 2024 01:05
@picnixz
Copy link
Contributor

picnixz commented Dec 14, 2024

At least, I think we should deprecate (not soft) this

I would be fine with deprecating it, saying which alternative to use, so that we can simply remove it in some later versions. I think Victor was the one who removed and restored it so we should ask him as well.

@picnixz
Copy link
Contributor

picnixz commented Dec 14, 2024

should dec_from_long() be modified here? (To use the PyLong_Export API.) I would prefer to do this in a separate PR.

If you prefer doing it in a follow-up PR because you fear it would be too hard to review, then it's better. If the change is minimal, we can do it this one (I didn't check the code to change)

@skirpichev
Copy link
Member Author

If the change is minimal, we can do it this one

You can estimate them looking on the gmpy2 pr (referenced in the PEP): aleaxit/gmpy#495 In principle, I don't think that this will complicate review to much. On another hand, changes looks logically independent. I would rather include here deprecation.

@picnixz
Copy link
Contributor

picnixz commented Dec 14, 2024

  • Let's change dec_from_long in another PR since the changes are independent (sorry it's 3 AM here and I don't have much energy).
  • For deprecating _PyLong_FromDigits, maybe it's better to make a separate PR so that we have a dedicated NEWS entry and re-use the issue that actually removed the private API (and not the issue that reverted the removal). WDYT? (we would also be able to change PyLong_Copy accordingly)

@skirpichev

This comment was marked as outdated.

@skirpichev skirpichev marked this pull request as draft December 14, 2024 05:07
@skirpichev skirpichev changed the title gh-102471: convert decimal module to use PyLongWriter API (PEP 757) gh-102471: convert decimal module to use import/export API for ints (PEP 757) Dec 14, 2024
@skirpichev skirpichev requested a review from picnixz December 14, 2024 06:53
@skirpichev skirpichev marked this pull request as ready for review December 14, 2024 07:10
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
* cleanup: forgotten PyLongWriter_Discard, pylong variable
* clarify news
@serhiy-storchaka
Copy link
Member

The precondition is still in the docs. It says MUST.

I've added asserts to ensure that no reallocation occurs.

I meant that there should be comments and asserts in the libmpdec. Testing that no reallocation occurs after the call is too late -- the program can already be crashed. And you cannot test for resizing if it occurs in-place, but the memory management structure can already be broken, and crash later.

For now we cannot use this code. If the libmpdec developers give satisfying guarantees, we could.

vstinner

This comment was marked as resolved.

@skirpichev
Copy link
Member Author

The precondition is still in the docs. It says MUST.

Docs also says that no memory management (read: resize) happens in our scenario. Do we agree on this?

And you cannot test for resizing if it occurs in-place

Added asserts rather for documentation, to show that qexport functions are used in a special way. Now comment added too.

If the libmpdec developers give satisfying guarantees

I think they already did this in docs.

Resize occur e.g. here:


That condition can be true only if wlen was underestimated. It can't happen if wlen was obtained by a call to mpd_sizeinbase, just as docs says.

Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
* move comment up
* move assert down
* remove redundant assert & restore nonzero assert
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. But I would prefer a second review, from @serhiy-storchaka or @picnixz for example :-)

@vstinner
Copy link
Member

Would you mind to share your benchmark code?

@skirpichev
Copy link
Member Author

But I would prefer a second review, from @serhiy-storchaka

Currently Serhiy clearly -1 on this. He think that we could be unsafe, because PyLongObject's and libmpdec use different memory management functions. See e.g. this. My point was that mpd_qexport*() functions should do no memory management at all with given arguments (len coming from mpd_sizeinbase). Do you agree with this or documentation is not clear for you on this aspect?

If neither you or @picnixz agree on above point - probably I should go to mpdecimal mailing list for a clarification.

Would you mind to share your benchmark code?

Ah, this was in "details" of the pr description:-)

@vstinner
Copy link
Member

Currently Serhiy clearly -1 on this. He think that we could be unsafe, because PyLongObject's and libmpdec use different memory management functions. See e.g. #127925 (comment). My point was that mpd_qexport*() functions should do no memory management at all with given arguments (len coming from mpd_sizeinbase). Do you agree with this or documentation is not clear for you on this aspect?

I don't think that the current implementation pass a pointer to the start of a memory block allocated by libmpdec. I know PEP 757 internals, and this change does basically exactly the same than the current code. It pass a pointer to PyLongObject.ob_digit. I'm fine with that.

@vstinner
Copy link
Member

I ran the benchmark with CPU isolation, Python built with gcc -O3.

Benchmark ref change
export Decimal(1<<3000) 60.0 us 52.5 us: 1.14x faster
import int(Decimal(1<<7)) 124 ns 91.0 ns: 1.37x faster
import int(Decimal(1<<38)) 127 ns 90.9 ns: 1.39x faster
import int(Decimal(1<<300)) 663 ns 733 ns: 1.10x slower
import int(Decimal(1<<3000)) 53.4 us 61.3 us: 1.15x slower
Geometric mean (ref) 1.07x faster

Benchmark hidden because not significant (3): export Decimal(1<<7), export Decimal(1<<38), export Decimal(1<<300)

  • Performance for integers up to 64-bit: neutral or up to 1.4x faster
  • Performance for large integers: export is faster (1.14x), import is slower (between 1.10x and 1.15x)

I didn't use PGO+LTO, maybe results are just pure noise. But it sounds unlikely that it's pure noise when the difference is at least 10% (1.10x).

@skirpichev
Copy link
Member Author

and this change does basically exactly the same than the current code

No! Code in the main branch pass ob_digit, which set to NULL. In that case mpd_qexport_*() functions do memory allocation and set this pointer. Then in _PyLong_FromDigits() this array memcpy'ed to digits of the PyLongObject instance and we do mpd_free(ob_digit).

New code pass non-NULL pointer to mpd_qexport_*(), it points to pre-allocated (by PyLongWriter_Create()) memory. It's a different case. From mpdecimal docs:

size_t mpd_qexport_u32(uint32_t *rdata, size_t rlen, uint32_t rbase, const mpd_t *src, uint32_t *status);
...
If rdata is non-NULL, it MUST be allocated by one of libmpdec’s allocation functions and rlen MUST be correct. If necessary, the function will resize rdata. Resizing is slow and should not occur if rlen has been obtained by a call to mpd_sizeinbase. In case of an error the caller must free rdata.

So, from my understanding, docs says us that no memory management (resizing) happens in mpd_qexport_*() call. Hence, precondition "MUST be allocated by one of libmpdec’s allocation functions" is irrelevant in our case.

@vstinner
Copy link
Member

Oh ok, thanks for the explanation.

@skirpichev
Copy link
Member Author

Performance for large integers: export is faster (1.14x), import is slower (between 1.10x and 1.15x)

Hmm, this looks strange for me. I did tests on a somewhat noisy system (just with "hands off keyboard"), but the difference with your case looks huge here.

@skirpichev skirpichev requested a review from picnixz December 22, 2024 12:52
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

On my side, here are the benchmarks with a release build (no PGO). I would like to mention that my machine is quite powerful and that sys.int_info[:2] == (30, 4) for me as well.

Specs

OS: openSUSE Leap 15.5 x86_64
Host: ROG Strix G814JZ_G814JZ 1.0
Kernel: 5.14.21-150500.55.83-default
CPU: 13th Gen Intel i9-13980HX (32) @ 5.400GHz
GPU: NVIDIA GeForce RTX 4080 Max-Q / Mobile
GPU: Intel Raptor Lake-S UHD Graphics
Memory: 11782MiB / 31698MiB

Export (int to Decimal)

+-----------------+------------+-----------------------+
| Benchmark       | export-ref | export-pep            |
+=================+============+=======================+
| Decimal(1<<38)  | 74.8 ns    | 72.0 ns: 1.04x faster |
+-----------------+------------+-----------------------+
| Decimal(1<<300) | 153 ns     | 161 ns: 1.06x slower  |
+-----------------+------------+-----------------------+
| Geometric mean  | (ref)      | 1.00x slower          |
+-----------------+------------+-----------------------+

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

Import (Decimal to int)

+-----------------------+------------+-----------------------+
| Benchmark             | import-ref | import-pep            |
+=======================+============+=======================+
| int(Decimal(1<<7))    | 61.8 ns    | 51.6 ns: 1.20x faster |
+-----------------------+------------+-----------------------+
| int(Decimal(1<<38))   | 74.4 ns    | 52.5 ns: 1.42x faster |
+-----------------------+------------+-----------------------+
| int(Decimal(1<<300))  | 138 ns     | 134 ns: 1.03x faster  |
+-----------------------+------------+-----------------------+
| int(Decimal(1<<3000)) | 7.26 us    | 7.30 us: 1.01x slower |
+-----------------------+------------+-----------------------+
| Geometric mean        | (ref)      | 1.15x faster          |
+-----------------------+------------+-----------------------+

Modules/_decimal/_decimal.c Show resolved Hide resolved
Modules/_decimal/_decimal.c Show resolved Hide resolved
Modules/_decimal/_decimal.c Show resolved Hide resolved
@picnixz
Copy link
Contributor

picnixz commented Dec 22, 2024

Ah I missed your question about mpd. Wait a bit until I've written my answer sorry.

@picnixz
Copy link
Contributor

picnixz commented Dec 22, 2024

My point was that mpd_qexport*() functions should do no memory management at all with given arguments (len coming from mpd_sizeinbase)

How I understand the docs you quoted:

If rdata is non-NULL, it MUST be allocated by one of libmpdec’s allocation functions and rlen MUST be correct

is that we should use mpd_mallocfunc or mpd_alloc. However, since mpd_mallocfunc is malloc by default, it doesn't change anything. However, I would like confirmation from libmpd maintainers that mpd_qexport would not do anything if we correctly allocate the memory that is being used (namely, mpd_qexport just uses the memory as is, and neither does it free it afterwards or steals the memory itself).

What I want to know is that we are allowed to use another allocation function which allocates the correct amount of memory. Namely that mpd_qexport is essentially equivalent to something like this:

def mpd_qexport(res, n, ...):
	if is_null(res):
		res, n = allocate(...)
	else:
		if should_resize(res, n):
			n = do_resize_and_compute_new_n(res, n)
	export_to_res(res, n)

In our case, I expect that we're bypassing all checks. But I'd like to know if the export_to_res subroutine has assumptions on whether the destination has been allocated using a mpd function or if allocating using malloc is compatible.

@skirpichev
Copy link
Member Author

However, since mpd_mallocfunc is malloc by default

It's overridden in the decimal module to ise PyMem_Malloc(). But _PyLong_New() uses PyObject_Malloc().

In our case, I expect that we're bypassing all checks.

Do you agree that follows unambiguously from the documentation? I.e. there should be no memory allocation and no resize.

I'd like to know if the export_to_res subroutine has assumptions on whether the destination has been allocated using a mpd function

But then this function just export data to a contiguous array. Why do you think it might be important how this array was allocated?

PS: Your benchmarks looks more close to mine than to Victor's.

@picnixz
Copy link
Contributor

picnixz commented Dec 22, 2024

Do you agree that follows unambiguously from the documentation? I.e. there should be no memory allocation and no resize.

That's how I understand it.

Why do you think it might be important how this array was allocated?

Well... considering they said "MUST", maybe there are some internals that I'm not aware of.

PS: Your benchmarks looks more close to mine than to Victor's.

Yes, sorry I forgot to say it. I also had "hands off" benchmarks rather than with CPU isolation so maybe there is something happening.

@skirpichev
Copy link
Member Author

Well... considering they said "MUST", maybe there are some internals that I'm not aware of.

I can't imagine some sane interpretation of mpdecimal internals where that might be essential.

Ok, I'll have to post in mpdecimal mail lists some stupid questions :-( Unless Serhiy changed his mind, this is no-go for now.

I also had "hands off" benchmarks rather than with CPU isolation so maybe there is something happening.

Maybe it's just a typo;)

@skirpichev
Copy link
Member Author

Here is reply from @skrah in the mpdecimal-discuss mail list:

mpd_sizeinbase() uses log10() from math.h for performance reasons. If log10() is IEEE compliant, the result should be sufficiently large. Resizing is for guarding against broken log10() implementations.

The current code in _decimal.c sets the libmpdec allocation functions to PyMem_Malloc() etc. So if longobject uses PyMem_Free() it is safe even when resizing occurs.

If the new API allows for allocators other that PyMem_Malloc(), it will rely on the IEEE compliance of log10().

Stefan Krah

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.

4 participants