No grow necessary in mp_set_int* functions #221

nijtmans · 2019-04-12T13:36:27Z

In the mp_set_long_long (and related functions), an mp_grow() call is done, but that's only necessary if the already allocated room (MP_PREC) is so small that not even 64 bits fit. There is only one such a situation: -DMP_LOW_MEM in combination with -DMP_8BIT.

Therefore, I propose to increase the minimum MP_PREC from 8 to 16 in this case (allocating 8 bytes is ridicilously small anyway), then the mp_grow() is no longer necessesary in any mode.

This means that the minimum number of bytes allocated are always minimum 16, which is always sufficient to store the 64 bits of a long long.

minad · 2019-04-12T13:48:55Z

Most allocators also don't return less than 16 bytes.

sjaeckel · 2019-04-12T13:51:16Z

thou shalt then never call mp_shrink before that :)

minad · 2019-04-12T13:53:32Z

mp_shrink should never shrink below this minimum precision? This should also be included here.
@sjaeckel Can it happen that numbers are completely empty or are they always allocated?

sjaeckel · 2019-04-12T13:57:34Z

I'd say it should be enough to never shrink below the required storage for a long long then! otherwise the target of mp_shrink isn't fulfilled anymore

sjaeckel · 2019-04-12T13:58:07Z

@sjaeckel Can it happen that numbers are completely empty or are they always allocated?

they're always allocated

Edit: btw I've killed the build as this requires more changes

nijtmans · 2019-04-12T14:03:59Z

Yeah, I found mp_shrink() as well ;-(

So mp_shrink() should be modified to always shrink to at least MP_PREC, and also always a multiple of that.

And .. mp_init_size/mp_grow lied: The comment said they always allocote MP_PREC digits extra, but in fact they always allocate MP_PREC+1 digits extra. That's too much. Fixed that as well ....

sjaeckel · 2019-04-12T14:09:04Z

So mp_shrink() should be modified to always shrink to at least MP_PREC, and also always a multiple of that.

I'd say it should be enough to never shrink below the required storage for a long long then

not MP_PREC

nijtmans · 2019-04-12T14:34:26Z

Hoping it's better now!

sjaeckel · 2019-04-12T14:51:23Z

can you please fix the commit message?

sjaeckel · 2019-04-12T14:53:23Z

bn_mp_shrink.c

@@ -7,20 +7,21 @@
 int mp_shrink(mp_int *a)
 {
   mp_digit *tmp;
-   int used = 1;
+   int alloc = MP_PREC;


this still shrink's only down to MP_PREC and not the required storage for a long long

I think the requirement of MP_PREC should be that it can store at least a long long. That's what the change in tommath.h was meant for! mp_init() also starts with allocating MP_PREC digits.

So, for MP_8BIT, the minimum MP_PREC=16, for MP_16BIT, the minimum MP_PREC=8, for MP_32BIT the minimum MP_PREC=4, for MP_64BIT it is MP_PREC=2. That should be documented somewhere.

@czurnieden what do you say?

What about adding a MP_MIN_PREC setting to the minimums given by @nijtmans and changing MP_PREC such that it is always larger than MP_MIN_PREC?
Furthermore I think MP_PREC is usually too large. But this depends heavly on the use case. Via static_assert it could be checked that the condition always holds if MP_PREC is set by the user.
Maybe just set MP_MIN_PREC to 16 / sizeof (mp_digit)?

How about an assert somewhere in the code checking that MP_PREC * sizeof(mp_digit) >= sizeof(long long) ???

Hm... I meant MP_PREC * DIGIT_BIT >= sizeof(long long). That means that MP_PREC for MP_8BIT could even be as low as 10, although that's not a power of 2.

Hm .... MP_PREC * DIGIT_BIT >= CHAR_BIT * sizeof(long long)

How about an assert somewhere in the code checking that MP_PREC * sizeof(mp_digit) >= sizeof(long long) ???

no we need to make it sure at compile time

I'd make it #define MP_MIN_PREC ((CHAR_BIT * sizeof(long long))/DIGIT_BIT) and hope that nobody uses this macro in a #if preprocessor statement :)

and please keep the patch to to tommath.h regarding MP_8BIT

czurnieden what do you say?

A MP_PREC that is always of the same bitsize no matter what the MP_xBITis would add nicely to the "rounding edges" part of the LTM update, yes.

bn_mp_grow.c

czurnieden

(pressed the wrong button again *grr*)
As hinted at in my other comment: if you find a way that MP_PREC/MP_MIN_PREC is of the same size , independent of MP_xBIT I would welcome this patch.

minad

I am generally very positive about these changes. It is much better if there are some reasonable guarantees of minimal int sizes. However I would like to see these extra size gone, to ensure all memory allocations reserve exactly what they need. This will expand the scope of this PR a bit, but it is worth it I think.

nijtmans · 2019-04-12T19:22:04Z

So, here my new attempt. MP_MIN_PREC is a multiple of MP_PREC such that at least a long long fits. So, we can set MP_PREC to 2, or even 1 if desired. All allocations will be a multiple of MP_PREC mp_digit's.

czurnieden · 2019-04-12T20:13:47Z

Yes, looks good!
But be aware that MP_PREC can be changed by the user and I think it would be helpful if the size of MP_PREC is checked. The preprocessor can do it and utter a #warning if e.g.: MP_PREC < 1. Typos are easily made and if it is equally easy to check them it should be done.

minad · 2019-04-27T16:11:49Z

@nijtmans @czurnieden What is the status here?

nijtmans · 2019-04-27T16:41:38Z

Well, I'm on vacation, so I can't do anything now. I'll have a look when I'm back

minad · 2019-04-27T17:03:39Z

@nijtmans Ok 👍

minad · 2019-05-12T13:54:45Z

close this in favor of #253

nijtmans force-pushed the no_grow_in_set_int branch from 156ab13 to 709b708 Compare April 12, 2019 14:32

nijtmans requested review from minad and sjaeckel April 12, 2019 14:34

sjaeckel reviewed Apr 12, 2019

View reviewed changes

No grow necessary in mp_set_int* functions

b83b6b9

nijtmans force-pushed the no_grow_in_set_int branch from 709b708 to b83b6b9 Compare April 12, 2019 14:55

nijtmans requested a review from czurnieden April 12, 2019 15:12

czurnieden reviewed Apr 12, 2019

View reviewed changes

bn_mp_grow.c Outdated Show resolved Hide resolved

czurnieden reviewed Apr 12, 2019

View reviewed changes

minad reviewed Apr 12, 2019

View reviewed changes

New macro: MP_MIN_PREC

46f6309

nijtmans added 2 commits April 12, 2019 21:39

Fix gcc warning and typo

72bb8fd

Now without unneeded extra's

65c07ec

This was referenced Apr 16, 2019

Resolving problems mentioned in #218 #219

Merged

more linting #218

Merged

minad mentioned this pull request May 8, 2019

API discussions/overhaul #243

Closed

This was referenced May 10, 2019

No grow in mp_set_int* (2) #252

Closed

No grow in mp_set_int (2) #253

Merged

minad closed this May 12, 2019

nijtmans deleted the no_grow_in_set_int branch July 5, 2019 23:27

No grow necessary in mp_set_int* functions #221

No grow necessary in mp_set_int* functions #221

Uh oh!

Conversation

nijtmans commented Apr 12, 2019

Uh oh!

minad commented Apr 12, 2019

Uh oh!

sjaeckel commented Apr 12, 2019

Uh oh!

minad commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaeckel commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaeckel commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nijtmans commented Apr 12, 2019

Uh oh!

sjaeckel commented Apr 12, 2019

Uh oh!

nijtmans commented Apr 12, 2019

Uh oh!

sjaeckel commented Apr 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minad Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

czurnieden left a comment

Choose a reason for hiding this comment

Uh oh!

minad left a comment

Choose a reason for hiding this comment

Uh oh!

nijtmans commented Apr 12, 2019

Uh oh!

czurnieden commented Apr 12, 2019

Uh oh!

minad commented Apr 27, 2019

Uh oh!

nijtmans commented Apr 27, 2019

Uh oh!

minad commented Apr 27, 2019

Uh oh!

minad commented May 12, 2019

Uh oh!

Uh oh!

minad commented Apr 12, 2019 •

edited

Loading

sjaeckel commented Apr 12, 2019 •

edited

Loading

sjaeckel commented Apr 12, 2019 •

edited

Loading

minad Apr 12, 2019 •

edited

Loading