search

vermaseren / form

The FORM project for symbolic manipulation of very big expressions
GNU General Public License v3.0
1.15k stars 136 forks source link

Split the large sort allocation into separate allocations #529

Open jodavies opened 4 months ago

jodavies commented 4 months ago

Here I have split the large single allocation in AllocSort into individual allocations. Only Large+Small(+extension) must be contiguous. The aim is that overrunning any of these buffers will cause a crash and not silent corruption of the program state.

In principle this has a performance penalty, though whether it is measurable or not has to be determined by benchmarking.

The second commit adds warning messages, if any of the buffers are adjusted from their requested values. I have changed some of the default (64bit) allocations, such that they are consistent with FORM's own requirements and no warnings are printed. This is not completely resolved yet: the sublargesize is still shifted, and more things are shifted in 32bit mode.

coveralls commented 4 months ago

Coverage Status

coverage: 49.985% (-0.01%) from 49.999% when pulling 30891c4b2d29a026ee62ad476955da554ec32ab1 on jodavies:allocations into 83e3d4185efca2e5938c665a6df9d67d6d9492ca on vermaseren:master.

jodavies commented 4 months ago

The second point is a bit tricky to resolve, since there is some circular interaction between largesize and IOsize.

I am not sure of the current code: https://github.com/vermaseren/form/blob/83e3d4185efca2e5938c665a6df9d67d6d9492ca/sources/setfile.c#L884-L903 IObuffersize is in units of WORD (L884). Then comes some code which checks (LargeSize+SmallEsize) against MaxFpatches*IObuffersize (with a constant offset). If it is too small, the large (or small) buffer are increased in size accordingly. This code runs, basically, if the user has set sortiosize significantly larger.

But then we make the same check in reverse, and adjust IObuffersize accordingly. This check seems broken though; IOtry in units of WORD is compared against IObuffersize in bytes. If I fix this comparison, the default setup parameters need more changes to satisfy the constraints. This "units mismatch" came in 8be528b.

jodavies commented 3 months ago

I have cleaned this up a bit, but now I have a few questions:

In RecalcSetups, the buffer size constraints are not consistent with those in AllocSort. AllocSort only allocates the internally-determined sizes, so it seems like this doesn't matter in the end. But I am not sure of the purpose of RecalcSetupts? https://github.com/vermaseren/form/blob/83e3d4185efca2e5938c665a6df9d67d6d9492ca/sources/setfile.c#L357

In AllocSetups the ziobuffer size is set to IOsize, but this value is modified inside AllocSort. This means that the ziobuffer is not the same size as POsize (it is smaller, but it is still at least MaxTermSize). Could this ever cause a problem? https://github.com/vermaseren/form/blob/83e3d4185efca2e5938c665a6df9d67d6d9492ca/sources/setfile.c#L567-L592

jodavies commented 3 months ago

Another thing I noticed: AllocSort allocates the SplitScratch array of TermsInSmall/2 pointers for use in SplitMerge. But actually this is never used; SplitMerge uses the SplitScratch arrays in the N struct. The array in the sort struct can be removed.