From b3f84b495b60cecc018f24f4162c8961c60ec819 Mon Sep 17 00:00:00 2001 From: Josh Davies Date: Sat, 4 May 2024 13:51:09 +0100 Subject: [PATCH 1/4] Better errors when (Sub)SmallExtension is filled. Previously, FORM warns only about SmallExtension, even if it is a sub-buffer sort. This prompts the user to adjust the wrong buffer in the setup. --- check/fixes.frm | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ sources/sort.c | 14 ++++++++++-- 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/check/fixes.frm b/check/fixes.frm index b14265fc..2af4e930 100644 --- a/check/fixes.frm +++ b/check/fixes.frm @@ -2841,6 +2841,65 @@ print; assert succeeded? assert result("F") =~ expr("5000") *--#] Issue508 : +*--#[ Issue512_1 : +#- +* Sort which fills SmallExtension: + +#: SmallSize 1000K +#: SmallExtension 1200K +#: SubSmallSize 1000K +#: SubSmallExtension 1200K +#: SubTermsInSmall 2M + +Symbol x,n; +CFunction f,g,prf; + +Local test = (+...+)*(+...+); +.sort + +Off parallel; +PolyRatFun prf; + +Identify f(x?) = prf(n-x,n+x); + +.end +# Fails due to polynomial size on 32bit builds +#require wordsize >= 4 +# Runtime errors may freeze ParFORM. +#pend_if mpi? +assert runtime_error?("Please increase SmallExtension setup parameter.") +*--#] Issue512_1 : +*--#[ Issue512_2 : +#- + +* Sort which fills SubSmallExtension: + +#: SmallSize 1000K +#: SmallExtension 1200K +#: TermsInSmall 2M +#: SubSmallSize 1000K +#: SubSmallExtension 1200K +#: SubTermsInSmall 2M + +Symbol x,n; +CFunction f,g,prf; + +Local test = 1; +.sort + +PolyRatFun prf; +Term; + Multiply (+...+)*(+...+); + Identify f(x?) = prf(n-x,n+x); +EndTerm; + +.end +# Fails due to polynomial size on 32bit builds +#require wordsize >= 4 +# Runtime errors may freeze ParFORM. +#pend_if mpi? +assert runtime_error?("Please increase SubSmallExtension setup parameter.") +*--#] Issue512_2 : *--#[ Issue525 : #:threadbucketsize 5 #:processbucketsize 5 diff --git a/sources/sort.c b/sources/sort.c index 3835d757..61ec3856 100644 --- a/sources/sort.c +++ b/sources/sort.c @@ -2234,11 +2234,21 @@ WORD AddPoly(PHEAD WORD **ps1, WORD **ps2) TalToLine((UWORD)(*s2++)); TokenToLine((UBYTE *)" "); } FiniLine(); - MesPrint("Please increase SmallExtension in %s",setupfilename); + if ( AR.sLevel > 0 ) { + MesPrint("Please increase SubSmallExtension setup parameter."); + } + else { + MesPrint("Please increase SmallExtension setup parameter."); + } MUNLOCK(ErrorMessageLock); #else MLOCK(ErrorMessageLock); - MesPrint("Please increase SmallExtension in %s",setupfilename); + if ( AR.sLevel > 0 ) { + MesPrint("Please increase SubSmallExtension setup parameter."); + } + else { + MesPrint("Please increase SmallExtension setup parameter."); + } MUNLOCK(ErrorMessageLock); #endif Terminate(-1); From c9d5abd6315f0db32e6578f7d9072c36d3b2d661 Mon Sep 17 00:00:00 2001 From: Josh Davies Date: Tue, 4 Jun 2024 16:00:58 +0100 Subject: [PATCH 2/4] Put pointer-zeroing back into SplitMerge, so GarbHand works Commit f1b83ae removed various zeroed pointers from SplitMerge, which does not produce wrong results, since they are "out of range" of the sorted and merged region of the pointer array, but it leads to GarbHand computing the total size of the terms incorrectly. This causes valgrind errors when we write over the end of the SmallExtension, and causes termination due to a "too small SmallExtension" which might not actually be the case. --- check/fixes.frm | 55 ++++++++++++++++++++++++++++ sources/sort.c | 96 +++++++++++++++++++++++++++++++------------------ 2 files changed, 116 insertions(+), 35 deletions(-) diff --git a/check/fixes.frm b/check/fixes.frm index 2af4e930..c6a52c76 100644 --- a/check/fixes.frm +++ b/check/fixes.frm @@ -2900,6 +2900,61 @@ EndTerm; #pend_if mpi? assert runtime_error?("Please increase SubSmallExtension setup parameter.") *--#] Issue512_2 : +*--#[ Issue512_3 : +#- + +* Sort which fits in SmallExtension, but needs GarbHand + +#: SmallSize 1000K +#: SmallExtension 5090K +#: TermsInSmall 2M +#: SubSmallSize 1000K +#: SubSmallExtension 1200K +#: SubTermsInSmall 2M + +Symbol x,n; +CFunction f,g,prf; + +Local test = (+...+)*(+...+); +.sort + +PolyRatFun prf; +Identify f(x?) = prf(n-x,n+x); + +.end +# Fails due to polynomial size on 32bit builds +#require wordsize >= 4 +assert succeeded? +*--#] Issue512_3 : +*--#[ Issue512_4 : +#- + +* Sort which fits in SubSmallExtension, but needs GarbHand + +#: SmallSize 1000K +#: SmallExtension 1200K +#: TermsInSmall 2M +#: SubSmallSize 1000K +#: SubSmallExtension 5090K +#: SubTermsInSmall 2M + +Symbol x,n; +CFunction f,g,prf; + +Local test = 1; +.sort + +PolyRatFun prf; +Term; + Multiply (+...+)*(+...+); + Identify f(x?) = prf(n-x,n+x); +EndTerm; + +.end +# Fails due to polynomial size on 32bit builds +#require wordsize >= 4 +assert succeeded? +*--#] Issue512_4 : *--#[ Issue525 : #:threadbucketsize 5 #:processbucketsize 5 diff --git a/sources/sort.c b/sources/sort.c index 61ec3856..7858fc70 100644 --- a/sources/sort.c +++ b/sources/sort.c @@ -3315,7 +3315,7 @@ LONG SplitMerge(PHEAD WORD **Pointer, LONG number) { GETBIDENTITY SORTING *S = AT.SS; - WORD **pp3, **pp1, **pp2; + WORD **pp3, **pp1, **pp2, **pptop; LONG i, newleft, newright, split; if ( number < 2 ) return(number); @@ -3325,12 +3325,13 @@ LONG SplitMerge(PHEAD WORD **Pointer, LONG number) pp3 = (WORD **)(*pp1); *pp1 = *pp2; *pp2 = (WORD *)pp3; } else if ( i == 0 ) { - number--; - if ( S->PolyWise ) { if ( AddPoly(BHEAD pp1,pp2) == 0 ) number = 0; } - else { if ( AddCoef(BHEAD pp1,pp2) == 0 ) number = 0; } + number--; + if ( S->PolyWise ) { if ( AddPoly(BHEAD pp1,pp2) == 0 ) number = 0; } + else { if ( AddCoef(BHEAD pp1,pp2) == 0 ) number = 0; } } return(number); } + pptop = Pointer + number; split = number/2; newleft = SplitMerge(BHEAD Pointer,split); newright = SplitMerge(BHEAD Pointer+split,number-split); @@ -3346,20 +3347,21 @@ LONG SplitMerge(PHEAD WORD **Pointer, LONG number) ( i = CompareTerms(BHEAD Pointer[newleft-1],Pointer[split],(WORD)0) ) >= 0 ) { pp2 = Pointer+split; pp1 = Pointer+newleft-1; if ( i == 0 ) { - if ( S->PolyWise ) { - if ( AddPoly(BHEAD pp1,pp2) > 0 ) pp1++; - else newleft--; - } - else { - if ( AddCoef(BHEAD pp1,pp2) > 0 ) pp1++; - else newleft--; - } - pp2++; newright--; + if ( S->PolyWise ) { + if ( AddPoly(BHEAD pp1,pp2) > 0 ) pp1++; + else newleft--; + } + else { + if ( AddCoef(BHEAD pp1,pp2) > 0 ) pp1++; + else newleft--; + } + *pp2++ = 0; newright--; } else pp1++; newleft += newright; if ( pp1 < pp2 ) { while ( --newright >= 0 ) *pp1++ = *pp2++; + while ( pp1 < pptop ) *pp1++ = 0; } return(newleft); } @@ -3367,56 +3369,80 @@ LONG SplitMerge(PHEAD WORD **Pointer, LONG number) if ( split >= AN.SplitScratchSize ) { AN.SplitScratchSize = (split*3)/2+100; if ( AN.SplitScratchSize > S->Terms2InSmall/2 ) - AN.SplitScratchSize = S->Terms2InSmall/2; + AN.SplitScratchSize = S->Terms2InSmall/2; if ( AN.SplitScratch ) M_free(AN.SplitScratch,"AN.SplitScratch"); AN.SplitScratch = (WORD **)Malloc1(AN.SplitScratchSize*sizeof(WORD *),"AN.SplitScratch"); } + pp3 = AN.SplitScratch; pp1 = Pointer; - for ( i = 0; i < newleft; i++ ) *pp3++ = *pp1++; + /* Move rather than copy, so GarbHand can't double-count. */ + for ( i = 0; i < newleft; i++ ) { *pp3++ = *pp1; *pp1++ = 0; } AN.InScratch = newleft; pp1 = AN.SplitScratch; pp2 = Pointer + split; pp3 = Pointer; + /* An improvement in the style of Timsort */ while ( newleft > 8 ) { + /* Check the middle of the LHS terms */ LONG nnleft = newleft/2; - if ( ( i = CompareTerms(BHEAD pp1[nnleft],*pp2,(WORD)0) ) < 0 ) break; - pp3 += nnleft+1; - pp1 += nnleft+1; - newleft -= nnleft+1; + if ( ( i = CompareTerms(BHEAD pp1[nnleft],*pp2,(WORD)0) ) < 0 ) { + /* The terms are not in order. Break out and continue as normal. */ + break; + } + /* The terms merge or are in order. Copy pointers up to this point. */ + /* In the copy, zero the skipped pointers so GarbHand can't double-count. */ + for (int iii = 0; iii < nnleft; iii++) { + *pp3++ = *pp1; + *pp1++ = 0; + } + newleft -= nnleft; if ( i == 0 ) { - if ( S->PolyWise ) { i = AddPoly(BHEAD pp3-1,pp2); } - else { i = AddCoef(BHEAD pp3-1,pp2); } - if ( i == 0 ) pp3--; - pp2++; + if ( S->PolyWise ) { i = AddPoly(BHEAD pp1,pp2); } + else { i = AddCoef(BHEAD pp1,pp2); } + if ( i == 0 ) { + /* The terms cancelled. The next term goes in *pp3. Don't move. */ + } + else { + /* The terms added. Advance pp3. */ + *pp3++ = *pp1; + } + /* We have taken a LHS (copy) and RHS term. */ + *pp2++ = 0; newright--; + *pp1++ = 0; + newleft--; break; } } while ( newleft > 0 && newright > 0 ) { if ( ( i = CompareTerms(BHEAD *pp1,*pp2,(WORD)0) ) < 0 ) { - *pp3++ = *pp2++; + *pp3++ = *pp2; + *pp2++ = 0; newright--; } else if ( i > 0 ) { - *pp3++ = *pp1++; + *pp3++ = *pp1; + *pp1++ = 0; newleft--; } else { - if ( S->PolyWise ) { if ( AddPoly(BHEAD pp1,pp2) > 0 ) *pp3++ = *pp1; } - else { if ( AddCoef(BHEAD pp1,pp2) > 0 ) *pp3++ = *pp1; } - pp1++; pp2++; newleft--; newright--; + if ( S->PolyWise ) { if ( AddPoly(BHEAD pp1,pp2) > 0 ) *pp3++ = *pp1; } + else { if ( AddCoef(BHEAD pp1,pp2) > 0 ) *pp3++ = *pp1; } + *pp1++ = 0; *pp2++ = 0; newleft--; newright--; } } - for ( i = 0; i < newleft; i++ ) *pp3++ = *pp1++; + for ( i = 0; i < newleft; i++ ) { *pp3++ = *pp1; *pp1++ = 0; } if ( pp3 == pp2 ) { pp3 += newright; } else { - for ( i = 0; i < newright; i++ ) *pp3++ = *pp2++; + for ( i = 0; i < newright; i++ ) { *pp3++ = *pp2++; } } + newleft = pp3 - Pointer; + while ( pp3 < pptop ) *pp3++ = 0; AN.InScratch = 0; - return(pp3 - Pointer); + return(newleft); } #else @@ -3461,7 +3487,7 @@ LONG SplitMerge(PHEAD WORD **Pointer, LONG number) if ( AddPoly(BHEAD pp1,pp2) > 0 ) pp1++; else newleft--; } - else { + else { if ( AddCoef(BHEAD pp1,pp2) > 0 ) pp1++; else newleft--; } @@ -3545,7 +3571,7 @@ VOID GarbHand(VOID) */ #ifdef TESTGARB MLOCK(ErrorMessageLock); - MesPrint("in: S->sFill = %x, S->sTop2 = %x",S->sFill,S->sTop2); + MesPrint("in: S->sFill = %l, S->sTop2 = %l",S->sFill-S->sBuffer,S->sTop2-S->sBuffer); #endif Point = S->sPointer; k = S->sTerms; @@ -3558,7 +3584,7 @@ VOID GarbHand(VOID) if ( ( s2 = *Point++ ) != 0 ) { total += *s2; } } #ifdef TESTGARB - MesPrint("total = %l, nterms = %l",2*total,AN.InScratch); + MesPrint("total = %l, nterms = %l",total,AN.InScratch); MUNLOCK(ErrorMessageLock); #endif /* @@ -3618,7 +3644,7 @@ VOID GarbHand(VOID) S->sFill = s2; #ifdef TESTGARB MLOCK(ErrorMessageLock); - MesPrint("out: S->sFill = %x, S->sTop2 = %x",S->sFill,S->sTop2); + MesPrint("out: S->sFill = %l, S->sTop2 = %l",S->sFill-S->sBuffer,S->sTop2-S->sBuffer); if ( S->sFill >= S->sTop2 ) { MesPrint("We are in deep trouble"); } From 90761b27a08236eab8745990377ab6c98f126f08 Mon Sep 17 00:00:00 2001 From: Josh Davies Date: Mon, 18 Nov 2024 13:48:22 +0000 Subject: [PATCH 3/4] Debugging output for SplitMerge, for future debugging Optionally print the state of the pointer array on entry to SplitMerge, for ease of potential future debugging here. --- sources/sort.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/sources/sort.c b/sources/sort.c index 7858fc70..aa29571e 100644 --- a/sources/sort.c +++ b/sources/sort.c @@ -51,6 +51,12 @@ #define GZIPDEBUG */ #define NEWSPLITMERGE +/* Comment to turn off Timsort in SplitMerge for debugging: */ +#define NEWSPLITMERGETIMSORT +/* During SplitMerge, print pointer array state on entry. Very spammy, for debugging. */ +/* #define SPLITMERGEDEBUG */ +/* Debug printing for GarbHand */ +/* #define TESTGARB */ #include "form3.h" @@ -3318,6 +3324,21 @@ LONG SplitMerge(PHEAD WORD **Pointer, LONG number) WORD **pp3, **pp1, **pp2, **pptop; LONG i, newleft, newright, split; +#ifdef SPLITMERGEDEBUG + /* Print current array state on entry. */ + printf("%4ld: ", number); + for (int ii = 0; ii < S->sTerms; ii++) { + if ( (S->sPointer)[ii] ) { + printf("%4d ", (unsigned)(S->sPointer[ii]-S->sBuffer)); + } + else { + printf(".... "); + } + } + printf("\n"); + fflush(stdout); +#endif + if ( number < 2 ) return(number); if ( number == 2 ) { pp1 = Pointer; pp2 = pp1 + 1; @@ -3380,6 +3401,7 @@ LONG SplitMerge(PHEAD WORD **Pointer, LONG number) AN.InScratch = newleft; pp1 = AN.SplitScratch; pp2 = Pointer + split; pp3 = Pointer; +#ifdef NEWSPLITMERGETIMSORT /* An improvement in the style of Timsort */ @@ -3415,6 +3437,7 @@ LONG SplitMerge(PHEAD WORD **Pointer, LONG number) break; } } +#endif while ( newleft > 0 && newright > 0 ) { if ( ( i = CompareTerms(BHEAD *pp1,*pp2,(WORD)0) ) < 0 ) { @@ -3455,6 +3478,21 @@ LONG SplitMerge(PHEAD WORD **Pointer, LONG number) LONG nleft, nright, i, newleft, newright; WORD **pptop; +#ifdef SPLITMERGEDEBUG + /* Print current array state on entry. */ + printf("%4ld: ", number); + for (int ii = 0; ii < S->sTerms; ii++) { + if ( (S->sPointer)[ii] ) { + printf("%4d ", (unsigned)(S->sPointer[ii]-S->sBuffer)); + } + else { + printf(".... "); + } + } + printf("\n"); + fflush(stdout); +#endif + if ( number < 2 ) return(number); if ( number == 2 ) { pp1 = Pointer; pp2 = pp1 + 1; From 4411553846eb45137e3dcdaf7858ac9cae89690f Mon Sep 17 00:00:00 2001 From: Josh Davies Date: Mon, 18 Nov 2024 15:22:36 +0000 Subject: [PATCH 4/4] Test cases: fix buffer sizes to avoid warning messages. Make sure the tests run properly for tform -w4 also. (This makes the tests larger and more time consuming, also for form). Also fix the buffers for the Issue468 test. --- check/fixes.frm | 57 +++++++++++++++++++++---------------------------- 1 file changed, 24 insertions(+), 33 deletions(-) diff --git a/check/fixes.frm b/check/fixes.frm index c6a52c76..08c96d13 100644 --- a/check/fixes.frm +++ b/check/fixes.frm @@ -2725,9 +2725,10 @@ EOF #: SortIOSize 200K #: SubSortIOSize 200K -#: SubSmallSize 100K -#: SubSmallExtension 200K -#: SubTermsInSmall 5K +#: SubLargeSize 134400480 +#: SubSmallSize 12800016 +#: SubSmallExtension 19200032 +#: SubTermsInSmall 5008 #define N "30" @@ -2845,19 +2846,17 @@ assert result("F") =~ expr("5000") #- * Sort which fills SmallExtension: -#: SmallSize 1000K -#: SmallExtension 1200K -#: SubSmallSize 1000K -#: SubSmallExtension 1200K -#: SubTermsInSmall 2M +* These are the smallest buffer sizes that are OK for -w4 tform workers +#: SmallSize 10240064 +#: SmallExtension 15360096 +#: TermsInSmall 100K Symbol x,n; -CFunction f,g,prf; +CFunction g,f,prf; -Local test = (+...+)*(+...+); +Local test = (+...+)*(+...+); .sort -Off parallel; PolyRatFun prf; Identify f(x?) = prf(n-x,n+x); @@ -2873,13 +2872,11 @@ assert runtime_error?("Please increase SmallExtension setup parameter.") #- * Sort which fills SubSmallExtension: - -#: SmallSize 1000K -#: SmallExtension 1200K -#: TermsInSmall 2M -#: SubSmallSize 1000K -#: SubSmallExtension 1200K -#: SubTermsInSmall 2M +* These are the default sizes at the time of writing: +#: SubSmallSize 2560016 +#: SubSmallExtension 3840032 +* These are not default: +#: SubTermsInSmall 100K Symbol x,n; CFunction f,g,prf; @@ -2904,18 +2901,14 @@ assert runtime_error?("Please increase SubSmallExtension setup parameter.") #- * Sort which fits in SmallExtension, but needs GarbHand - -#: SmallSize 1000K -#: SmallExtension 5090K -#: TermsInSmall 2M -#: SubSmallSize 1000K -#: SubSmallExtension 1200K -#: SubTermsInSmall 2M +#: SmallSize 10240064 +#: SmallExtension 20360K +#: TermsInSmall 100K Symbol x,n; -CFunction f,g,prf; +CFunction g,f,prf; -Local test = (+...+)*(+...+); +Local test = (+...+)*(+...+); .sort PolyRatFun prf; @@ -2930,13 +2923,11 @@ assert succeeded? #- * Sort which fits in SubSmallExtension, but needs GarbHand - -#: SmallSize 1000K -#: SmallExtension 1200K -#: TermsInSmall 2M -#: SubSmallSize 1000K +* These are the default sizes at the time of writing: +#: SubSmallSize 2560016 +* These are not default: #: SubSmallExtension 5090K -#: SubTermsInSmall 2M +#: SubTermsInSmall 100K Symbol x,n; CFunction f,g,prf;