Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
Description
Feature or enhancement
Proposal:
After#127925, new code in the decimal module (using thePEP 757) do temporary allocation with mpdecimal's memory functions (in mpd_qexport_u32/16()), just as before:
cpython/Modules/_decimal/_decimal.c
Lines 3691 to 3706 inf10b7b2
| uint32_tbase= (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_tn; | |
| 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 tothe 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.
The thread on mpdecimal-discuss maillist:
https://lists.sr.ht/~mpdecimal/mpdecimal-discuss/%3CZ2kIike42Wl0_YKd@skirpichev.msk.ru%3E
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