Reorder methods after new implications are added

Many GAP methods get rank adjustments based on the rank of some filter,
e.g, `RankFilter(IsAbelian)`. However, these ranks may change when new
implications are added, e.g. when certain packages are loaded. But those
method rank adjustments then won't be updated, which can ultimately lead
to the effect that loading a package changes which methods get executed
in otherwise identical code.

This PR helps avoid that, by making it possible to set "dynamic" rank
adjustments which get updated as new implications are added. Some cleverness
is needed to make this work efficiently when loading packages.

- Record rank adjustment in METHODS_OPERATION
- Add RECALCULATE_ALL_METHOD_RANKS function which does what it says.
- Adjust MethodsOperation to include base rank
- Increase size of HIDDEN_IMPS_CACHE and WITH_IMPS_CACHE to speed up RECALCULATE_ALL_METHOD_RANKS()
- Make sure RANK_FILTERS is always set promptly, then remove the test for it being unset
  in RankFilter
- Automatically reorder methods after library or package loading, or InstallTrueMethod
- Use COPY_LIST_ENTRIES in method installation to avoid making an intermediate list
- Support passing a function of no arguments as the rank adjustment to InstallMethod
- Reset reordering status on quit from break loop.

Fixes #2513

doc/ref/methsel.xml

@@ -252,6 +252,7 @@ one can install also <E>immediate methods</E>.
 <Heading>Logical Implications</Heading>
 
 <#Include Label="InstallTrueMethod">
+<#Include Label="MethodReordering">
 
 </Section>
 

lib/error.g

@@ -32,6 +32,9 @@ BIND_GLOBAL( "OnQuit",  # care to ensure it always has a definition. - GG
       until IsEmpty(OptionsStack);
       Info(InfoWarning,1,"Options stack has been reset");
     fi;
+    if IsBound(ResetMethodReordering) and IsFunction(ResetMethodReordering) then
+        ResetMethodReordering();
+    fi;
 end);
 
 

lib/filter.g

@@ -171,6 +171,64 @@ BIND_GLOBAL( "InstallTrueMethodNewFilter", function ( tofilt, from )
 end );
 
 
+#############################################################################
+##
+#F  SuspendMethodReordering( )
+#F  ResumeMethodReordering( )
+#F  ResetMethodReordering( )
+##
+##  <#GAPDoc Label="MethodReordering">
+##  <ManSection>
+##  <Func Name="SuspendMethodReordering" Arg=""/>
+##  <Func Name="ResumeMethodReordering" Arg=""/>
+##  <Func Name="ResetMethodReordering" Arg=""/>
+##
+##  <Description>
+##  These functions control whether the method reordering process described
+##  in <Ref Func="InstallTrueMethod"/> is invoked or not. Since this process
+##  can be comparatively time-consuming, it is usually suspended when a lot
+##  of implications are due to be installed, for instance when loading the
+##  library, or a package. This is done by calling
+##  <C>SuspendMethodReordering()</C> once the installations are done,
+##  <C>ResumeMethodReordering()</C> should be called. These pairs of calls
+##  can be nested. When the outermost pair is complete, method reordering
+##  takes place and is enabled in <C>InstallTrueMethod</C> thereafter.
+##
+##  <C>ResetMethodReordering()</C> effectively exits all nested suspensions,
+##  resuming reordering immediately. This function is mainly provided for
+##  error recovery and similar purposes and is called on quitting from a
+##  break loop.
+##  </Description>
+##  </ManSection>
+##  <#/GAPDoc>
+
+#
+# This function will be defined in oper.g
+#
+RECALCULATE_ALL_METHOD_RANKS := fail;
+
+REORDER_METHODS_SUSPENSION_LEVEL := 1;
+
+BIND_GLOBAL( "SuspendMethodReordering", function()
+    REORDER_METHODS_SUSPENSION_LEVEL := REORDER_METHODS_SUSPENSION_LEVEL + 1;
+end);
+
+
+BIND_GLOBAL( "ResumeMethodReordering", function()
+    if REORDER_METHODS_SUSPENSION_LEVEL > 0 then
+        REORDER_METHODS_SUSPENSION_LEVEL := REORDER_METHODS_SUSPENSION_LEVEL - 1;
+    fi;
+    if REORDER_METHODS_SUSPENSION_LEVEL <= 0 then
+        RECALCULATE_ALL_METHOD_RANKS();
+    fi;
+end);
+
+BIND_GLOBAL( "ResetMethodReordering", function()
+    REORDER_METHODS_SUSPENSION_LEVEL := 0;
+    RECALCULATE_ALL_METHOD_RANKS();
+end);
+
+
 #############################################################################
 ##
 #F  InstallTrueMethod( <newfil>, <filt> )
@@ -210,17 +268,32 @@ end );
 ##  one can rely on the fact that every object in the filter
 ##  <C>IsGroup and IsCyclic</C> will also be in the filter
 ##  <Ref Func="IsCommutative"/>.
+##  <P/>
+##  Adding logical implications can change the rank of filters (see
+##  <Ref Func="RankFilter"/>) and consequently the rank, and so choice of
+##  methods for operations (see <Ref Sect="Applicable Methods and Method Selection"/>).
+##  By default <C>InstallTrueMethod</C> adjusts the method selection data
+##  structures to take care of this, but this process can be time-consuming,
+##  so functions <Ref Func="SuspendMethodReordering"/> and
+##  <Ref Func="ResumeMethodReordering"/> are provided to allow control of this process.
 ##  </Description>
 ##  </ManSection>
 ##  <#/GAPDoc>
 ##
+
 BIND_GLOBAL( "InstallTrueMethod", function ( tofilt, from )
 
     InstallTrueMethodNewFilter( tofilt, from );
 
     # clear the caches because we do not know if filter <from> is new
     CLEAR_HIDDEN_IMP_CACHE( from );
     CLEAR_IMP_CACHE();
+
+    # maybe rerank methods to take account of new implication
+    if REORDER_METHODS_SUSPENSION_LEVEL = 0 then
+        RECALCULATE_ALL_METHOD_RANKS();
+    fi;
+
 end );
 
 

lib/init.g

@@ -1008,6 +1008,9 @@ fi;
 ##
 ##  Read init files, run a shell, and do exit-time processing.
 ##
+
+ResumeMethodReordering();
+
 InstallAndCallPostRestore( function()
     local i, status;
     for i in [1..Length(GAPInfo.InitFiles)] do

lib/oper.g

@@ -1927,7 +1927,7 @@ end);
 
 fi;
 
-if BASE_SIZE_METHODS_OPER_ENTRY <> 5 then
+if BASE_SIZE_METHODS_OPER_ENTRY <> 6 then
     Error("MethodsOperation must be updated for new BASE_SIZE_METHODS_OPER_ENTRY");
 fi;
 
@@ -1948,6 +1948,7 @@ BIND_GLOBAL("MethodsOperation", function(oper, nargs)
             func    := meths[i + nargs + 2],
             rank    := meths[i + nargs + 3],
             info    := meths[i + nargs + 4],
+            rankbase := meths[i + nargs + 6],
             );
         ADD_LIST(result, m);
         if IsBound(meths[i + nargs + 5]) then
@@ -1957,6 +1958,133 @@ BIND_GLOBAL("MethodsOperation", function(oper, nargs)
     return result;
 end );
 
+#############################################################################
+##
+#F RECALCULATE_ALL_METHOD_RANKS() . . reorder methods after new implications
+##
+## Installing new implications (including hidden implications) can change the
+## rank of existing filters, and so of existing methods for operations.
+##
+## This function recalculates all such ranks and adjusts the method ordering
+## where needed. If the ordering changes, the relevant caches are flushed.
+##
+## If PRINT_REORDERED_METHODS is true, it prints some diagnostics (this is a
+## bit too low-level for Info).
+##
+##
+
+
+#
+# We had to install a placeholder for this in filter.g
+#
+Unbind(RECALCULATE_ALL_METHOD_RANKS);
+
+PRINT_REORDERED_METHODS := false;
+
+BIND_GLOBAL( "RECALCULATE_ALL_METHOD_RANKS", function()
+    local  oper, n, changed, meths, nmethods, i, base, rank, j, req,
+           req2, k, l;
+
+    for oper in OPERATIONS do
+        for n in [0..6] do
+            changed := false;
+            meths := METHODS_OPERATION(oper, n);
+            nmethods := LENGTH(meths)/(BASE_SIZE_METHODS_OPER_ENTRY+n);
+            if IS_CONSTRUCTOR(oper) and n > 0 then
+                for i in [nmethods,nmethods-1..1] do
+                    base := (i-1)*(BASE_SIZE_METHODS_OPER_ENTRY+n);
+                    # data for this method is meths{[base+1..base+BASE_SIZE_METHODS_OPER_ENTRY + n]}
+                    rank := meths[base+6+n];
+                    if IS_FUNCTION(rank) then
+                        rank := rank();
+                    fi;
+                    rank := rank - RankFilter(WITH_IMPS_FLAGS(meths[base+2]));
+                    if rank <> meths[base+n+3] then
+                        if IsHPCGAP and not changed then
+                            meths := SHALLOW_COPY_OBJ(meths);
+                        fi;
+                        changed := true;
+                        meths[base+n+3] := rank;
+                    fi;
+                    # compare to rank of succeding method
+                    if i = nmethods or rank >= meths[base+BASE_SIZE_METHODS_OPER_ENTRY + 2*n + 3] then
+                        continue;
+                    fi;
+                    k := i+2;
+                    while k <= nmethods and meths[(k-1)*(BASE_SIZE_METHODS_OPER_ENTRY+n) + n + 3] < rank do
+                        k := k+1;
+                    od;
+                    k := k-1;
+                    if PRINT_REORDERED_METHODS then
+                        Print("Constructor ",NAME_FUNC(oper), " ", n," args. Moving method ",i," (",
+                              meths[base+n+4]," from ",meths[base+n+5][1],":", meths[base+n+5][2],
+                              ") to position ",k,"\n");
+                    fi;
+                    l := meths{[base+1..base+n+BASE_SIZE_METHODS_OPER_ENTRY]};
+                    COPY_LIST_ENTRIES(meths, 1 + i*(n+BASE_SIZE_METHODS_OPER_ENTRY), 1,
+                            meths, 1 +(i-1)*(n+BASE_SIZE_METHODS_OPER_ENTRY), 1,
+                            (k-i)*(n+BASE_SIZE_METHODS_OPER_ENTRY));
+                    meths{[1 + (k-1)*(n+BASE_SIZE_METHODS_OPER_ENTRY)..
+                           k*(n+BASE_SIZE_METHODS_OPER_ENTRY)]} := l;
+                od;
+            else
+                for i in [1 ..nmethods] do
+                    base := (i-1)*(BASE_SIZE_METHODS_OPER_ENTRY+n);
+                    # data for this method is meths{[base+1..base+BASE_SIZE_METHODS_OPER_ENTRY + n]}
+                    rank := meths[base+6+n];
+                    if IS_FUNCTION(rank) then
+                        rank := rank();
+                    fi;
+
+
+                    for j in [1..n] do
+                        req := meths[base+1+j];
+                        rank := rank+RankFilter(WITH_IMPS_FLAGS(req));
+                    od;
+
+                    if rank <> meths[base+n+3] then
+                        if IsHPCGAP and not changed then
+                            meths := SHALLOW_COPY_OBJ(meths);
+                        fi;
+                        changed := true;
+                        meths[base+n+3] := rank;
+                    fi;
+
+                    # compare to rank of preceding method
+                    if i = 1 or rank <= meths[base-BASE_SIZE_METHODS_OPER_ENTRY+3] then
+                        continue;
+                    fi;
+
+                    k := i-2;
+                    while k > 1 and meths[(k-1)*(BASE_SIZE_METHODS_OPER_ENTRY+n) + n + 3] < rank do
+                        k := k-1;
+                    od;
+                    k := k+1;
+                    if PRINT_REORDERED_METHODS then
+                        Print(NAME_FUNC(oper), " ", n," args. Moving method ",i," (",
+                              meths[base+n+4]," from ",meths[base+n+5][1],":", meths[base+n+5][2],
+                              ") to position ",k,"\n");
+                    fi;
+                    l := meths{[base+1..base+n+BASE_SIZE_METHODS_OPER_ENTRY]};
+                    COPY_LIST_ENTRIES(meths, 1 + (k-1)*(n+BASE_SIZE_METHODS_OPER_ENTRY), 1,
+                            meths, 1 + k*(n+BASE_SIZE_METHODS_OPER_ENTRY), 1,
+                            (i-k)*(n+BASE_SIZE_METHODS_OPER_ENTRY));
+                    meths{[1 + (k-1)*(n+BASE_SIZE_METHODS_OPER_ENTRY)..
+                           k*(n+BASE_SIZE_METHODS_OPER_ENTRY)]} := l;
+                od;
+            fi;
+            if changed then
+                if IsHPCGAP then
+                    SET_METHODS_OPERATION(oper,n,MakeReadOnlySingleObj(meths));
+                else
+                    CHANGED_METHODS_OPERATION(oper,n);
+                fi;
+            fi;
+        od;
+    od;
+end );
+
+
 #############################################################################
 ##
 #E

lib/oper1.g

@@ -143,15 +143,20 @@ fi;
 #F  INSTALL_METHOD_FLAGS( <opr>, <info>, <rel>, <flags>, <rank>, <method> ) .
 ##
 BIND_GLOBAL( "INSTALL_METHOD_FLAGS",
-    function( opr, info, rel, flags, rank, method )
-    local   methods,  narg,  i,  k,  tmp, replace, match, j, lk;
+    function( opr, info, rel, flags, baserank, method )
+    local   methods,  narg,  i,  k,  tmp, replace, match, j, lk, rank;
 
     if IsHPCGAP then
         # TODO: once the GAP compiler supports 'atomic', use that
         # to replace the explicit locking and unlocking here.
         lk := WRITE_LOCK(METHODS_OPERATION_REGION);
     fi;
     # add the number of filters required for each argument
+    if IS_FUNCTION(baserank) then
+        rank := baserank();
+    else
+        rank := baserank;
+    fi;
     if IS_CONSTRUCTOR(opr) then
         if 0 = LEN_LIST(flags)  then
             Error(NAME_FUNC(opr),": constructors must have at least one argument");
@@ -262,6 +267,9 @@ BIND_GLOBAL( "INSTALL_METHOD_FLAGS",
     methods[i+(narg+4)] := IMMUTABLE_COPY_OBJ(info);
     if BASE_SIZE_METHODS_OPER_ENTRY >= 5 then
         methods[i+(narg+5)] := MakeImmutable([INPUT_FILENAME(), READEVALCOMMAND_LINENUMBER, INPUT_LINENUMBER()]);
+        if BASE_SIZE_METHODS_OPER_ENTRY >= 6 then
+            methods[i+(narg+6)] := baserank;
+        fi;
     fi;
 
     # flush the cache
@@ -290,9 +298,11 @@ end );
 ##  if supplied <A>info</A> should be a short but informative string
 ##  that describes for what situation the method is installed,
 ##  <A>famp</A> should be a function to be applied to the families
-##  of the arguments,
-##  and <A>val</A> should be an integer that measures the priority
-##  of the method.
+##  of the arguments.
+##  a<A>val</A> should be an integer that measures the priority
+##  of the method, or a function of no arguments which should return such an
+##  integer and will be called each time method order is being recalculated,
+##  (see <Ref Func="InstallTrueMethod"/>).
 ##  <P/>
 ##  The default values for <A>info</A>, <A>famp</A>, and <A>val</A> are
 ##  the empty string,
@@ -455,9 +465,11 @@ BIND_GLOBAL( "INSTALL_METHOD",
     # Check the rank.
     if not IsBound( arglist[ pos ] ) then
       Error( "the method is missing in <arglist>" );
-    elif IS_INT( arglist[ pos ] ) then
-      rank:= arglist[ pos ];
-      pos:= pos + 1;
+    elif IS_INT( arglist[ pos ] ) or
+         (IS_FUNCTION( arglist[ pos ] ) and NARG_FUNC( arglist[ pos ] ) = 0
+           and pos < LEN_LIST(arglist)) then
+        rank := arglist[ pos ];
+        pos := pos+1;
     else
       rank:= 0;
     fi;

lib/package.gi

@@ -1519,6 +1519,10 @@ InstallGlobalFunction( LoadPackage, function( arg )
       return path;
     fi;
 
+    # Suspend reordering of methods following InstallTrueMethod
+    # because it would slow things down too much
+    SuspendMethodReordering();
+
     # Compute the order in which the packages are loaded.
     # For each set of packages with cyclic dependencies,
     # we will first read all `init.g' files
@@ -1621,6 +1625,8 @@ InstallGlobalFunction( LoadPackage, function( arg )
     LogPackageLoadingMessage( PACKAGE_DEBUG, "return from LoadPackage",
         Name );
     GAPInfo.LoadPackageLevel:= GAPInfo.LoadPackageLevel - 1;
+
+    ResumeMethodReordering();
     return true;
     end );
 
@@ -1630,11 +1636,13 @@ InstallGlobalFunction( LoadPackage, function( arg )
 #F  LoadAllPackages()
 ##
 InstallGlobalFunction( LoadAllPackages, function()
+    SuspendMethodReordering();
     if ValueOption( "reversed" ) = true then
-    	List( Reversed( RecNames( GAPInfo.PackagesInfo ) ), LoadPackage );
+        List( Reversed( RecNames( GAPInfo.PackagesInfo ) ), LoadPackage );
     else
-    	List( RecNames( GAPInfo.PackagesInfo ), LoadPackage );
-    fi;	
+        List( RecNames( GAPInfo.PackagesInfo ), LoadPackage );
+    fi;
+    ResumeMethodReordering();
     end );