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

Allow to define custom MP_MIN_DIGIT_COUNT. #569

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

J08nY
Copy link
Contributor

@J08nY J08nY commented Nov 28, 2023

Basically, some internal functions ignore MP_DEFAULT_DIGIT_COUNT and allocate much smaller, which then reads to reallocations quite a bit.

Allowing a custom MP_MIN_DIGIT_COUNT is nice when one wants to save reallocations, also related to constant-timeness a bit (#567).

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I didn't think of that before and I believe immediately that this can hit performance.

Still I'm not sure whether that's the best approach!

There are two places where MP_MIN_DIGIT_COUNT are used, mp_init_size() and mp_shrink(). And mp_init_size() is the one that is used throughout the library.

Wouldn't it make more sense to change

size = MP_MAX(MP_MIN_DIGIT_COUNT, size);
to use MP_DEFAULT_DIGIT_COUNT instead?

Like that we could still shrink to the real minimum size and whenever mp_init_size() is called with a size < MP_DEFAULT_DIGIT_COUNT it would at least alloc that.

What do you think? That proposed modification should have the same results, right?

Also please squash fixup commits.

@J08nY
Copy link
Contributor Author

J08nY commented Nov 29, 2023

Hey there.

I went the way of allowing a custom MP_MIN_DIGIT_COUNT instead of modifying the uses in mp_init_size because I did not want to influence the default behavior of the library, but just allow me (and others that might find it useful) to modify it. The default behavior of the functions that use mp_init_size is likely right, they should know the appropriate size of the int they will need and so they allocate it. I guess even performance wise as it is now it may be better (depends on how you measure but it likely uses less memory as is).

My aim was to have libtommath with the least reallocations possible and with ints of a fixed size essentially. This is because I saw that the occasional mp_grow (and thus often a reallocation) caused significant timing differences that we do not want in our use of libtommath (and those timing differences also depended on the state of the allocator and the size of the alloc request).

With both the default and min digit count set to a sensible value (>= 2*field size for our ECC use case) we get way less reallocations and have more of fixed-size ints (though not really).

I am of course open to any change that gets this, I just don't want to be intrusive to other users of this library that might not want something like this.

@sjaeckel
Copy link
Member

sjaeckel commented Dec 5, 2023

@czurnieden your opinion on this?

@czurnieden
Copy link
Contributor

@czurnieden your opinion on this?

A custom MP_MIN_DIGIT_COUNT by way of preprocessor allows for negative values and therefore quite large ones—still smaller than MP_MAX_DIGIT_COUNT—if the type of size changes to size_t . Either catch that (e.g. by moving check for size < 0down after thesize = MAX(...)` ) or put a warning note in the documentation.

Otherwise: yes, good idea, has my support.

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

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

Can you please rebase on top of develop and squash those two commits.

@sjaeckel sjaeckel merged commit 138309c into libtom:develop Mar 12, 2024
79 checks passed
@sjaeckel sjaeckel added this to the v2.0.0 milestone Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants