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

Avoid temporary allocation in dec_as_long() #129275

Open
skirpichev opened this issue Jan 25, 2025 · 0 comments
Open

Avoid temporary allocation in dec_as_long() #129275

skirpichev opened this issue Jan 25, 2025 · 0 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@skirpichev
Copy link
Member

skirpichev commented Jan 25, 2025

Feature or enhancement

Proposal:

After #127925, new code in the decimal module (using the PEP 757) do temporary allocation with mpdecimal's memory functions (in mpd_qexport_u32/16()), just as before:

uint32_t base = (uint32_t)1 << layout->bits_per_digit;
/* We use a temporary buffer for digits for now, as for nonzero rdata
mpd_qexport_u32/u16() require either space "allocated by one of
libmpdec’s allocation functions" or "rlen MUST be correct" (to avoid
reallocation). This can be further optimized by using rlen from
mpd_sizeinbase(). See gh-127925. */
void *tmp_digits = NULL;
size_t n;
status = 0;
if (layout->digit_size == 4) {
n = mpd_qexport_u32((uint32_t **)&tmp_digits, 0, base, x, &status);
}
else {
n = mpd_qexport_u16((uint16_t **)&tmp_digits, 0, base, x, &status);
}

According to the documentation, we can prepare array of digits, which size could be estimated by mpd_sizeinbase(). Given this, the mpd_qexport_u32/16() shouldn't do any resize for this array. Here is Stefan comment on how safe this approach is:

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

We can expect slight performance boost for small (2-3 digits) integers.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

@skirpichev skirpichev added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Jan 25, 2025
@skirpichev skirpichev self-assigned this Jan 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

1 participant