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.

Sign up for GitHub

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

Jump to bottom

FIX: Bad Memory inefficiency in new MTC. #2812

Merged
merged 2 commits into from
Sep 27, 2018
Merged

FIX: Bad Memory inefficiency in new MTC. #2812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf7021a
FIX: Bad Memory inefficiency in new MTC.
hulpke Sep 14, 2018
78cabd9
ENHANCE: Better strategy for cyclic subgroup for fp group order
hulpke Sep 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions lib/grpfp.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3726,7 +3726,7 @@ end );
## enumerations with cumulatively bigger coset tables up to table size
## <maxtable>. It returns `fail' if no table could be found.
BindGlobal("FinIndexCyclicSubgroupGenerator",function(G,maxtable)
local fgens,grels,max,gens,t,Attempt;
local fgens,grels,max,gens,t,Attempt,perms,short;
fgens:=FreeGeneratorsOfFpGroup(G);
grels:=RelatorsOfFpGroup(G);
max:=ValueOption("max");
Expand All @@ -3742,6 +3742,7 @@ local fgens,grels,max,gens,t,Attempt;
#pseudorandom element - try if it works
PseudoRandom(G:radius:=Random(2,3))],
Filtered(gens,j->UnderlyingElement(j)<>UnderlyingElement(t)));
gens:=Set(List(gens,UnderlyingElement));

# recursive search (via smaller and smaller partitions) for a finite index
# subgroup
Expand All @@ -3753,7 +3754,7 @@ local fgens,grels,max,gens,t,Attempt;
trial:=sgens{[1..m]};
Info(InfoFpGroup,1,"FIS: trying ",trial);
t:=CosetTableFromGensAndRels(fgens,grels,
List(trial,UnderlyingElement):silent:=true,max:=max);
trial:silent:=true,max:=max);
if t<>fail and Length(trial)>1 then
Unbind(t);
t:=Attempt(trial);
Expand All @@ -3780,7 +3781,17 @@ local fgens,grels,max,gens,t,Attempt;
while max<=maxtable do
t:=Attempt(gens);
if t<>fail then
return t;
perms:=List(t[2]{[1,3..Length(t[2])-1]},PermList);
short:=FreeGeneratorsOfFpGroup(G);
short:=Concatenation(short, List(short,Inverse));
short:=Set(List(Concatenation(List([1..3],x->Arrangements(short,x))),
Product));
short:=List(short,
x->[Order(MappedWord(x,FreeGeneratorsOfFpGroup(G),perms)),x]);
Sort(short);
Info(InfoFpGroup,1,"FIS: better ",short[Length(short)][1]);
return [ElementOfFpGroup(FamilyObj(One(G)),short[Length(short)][2]),
max];
fi;
if max*3/2<maxtable and max*2>maxtable then
max:=maxtable;
Expand Down Expand Up @@ -3829,7 +3840,7 @@ local fgens, # generators of the free group
# the group could be quite big -- try to find a cyclic subgroup of
# finite index.
gen:=FinIndexCyclicSubgroupGenerator(G,infinity);
max:=gen[3];
max:=gen[2];
gen:=gen[1];

H := Subgroup(G,[gen]);
Expand Down
58 changes: 43 additions & 15 deletions lib/sgpres.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2372,10 +2372,10 @@ local c,o,n,j,au;
for j in DATA.A do
c[j+o][n]:=0;
od;
c[a+o][i]:=n;
c[o+a][i]:=n;
c[o-a][n]:=i;
if DATA.augmented then
DATA.aug[a+o][i]:=DATA.one;
DATA.aug[o+a][i]:=DATA.one;
DATA.aug[o-a][n]:=DATA.one;
DATA.pp[n]:=DATA.one;
fi;
Expand All @@ -2398,6 +2398,8 @@ end;
NEWTC_Coincidence:=function(DATA,a,b)
local Rep,Merge,ct,offset,l,q,i,c,x,d,p,mu,nu;

if a=b then return;fi;

Rep:=function(kappa)
local lambda,rho,mu;
lambda:=kappa;
Expand Down Expand Up @@ -2439,6 +2441,7 @@ local Rep,Merge,ct,offset,l,q,i,c,x,d,p,mu,nu;
for x in DATA.A do
if ct[x+offset][c]<>0 then
d:=ct[x+offset][c];
ct[x+offset][c]:=0;
ct[-x+offset][d]:=0;
mu:=Rep(c);
nu:=Rep(d);
Expand All @@ -2454,7 +2457,6 @@ local Rep,Merge,ct,offset,l,q,i,c,x,d,p,mu,nu;
fi;
od;
od;
#Error("coinc\n");
end;

NEWTC_ModifiedCoincidence:=function(DATA,a,b,w)
Expand All @@ -2474,11 +2476,9 @@ local MRep,MMerge,ct,offset,l,q,i,c,x,d,p,pp,mu,nu,aug,v,Sekundant;
local lambda,rho,mu,s;
lambda:=kappa;
rho:=p[lambda];
if rho=lambda then
return lambda;
fi;
if rho=lambda then return lambda; fi;

s:=[]; # new array to trace back compression path
s:=DATA.s; # re-used array to trace back compression path
while rho<>lambda do
s[rho]:=lambda;
lambda:=rho;rho:=p[lambda];
Expand Down Expand Up @@ -2682,10 +2682,8 @@ local c,offset,f,b,r,i,j,fp,bp,t;
c:=DATA.ct;
offset:=DATA.offset;
t:=TC_QUICK_SCAN(c,offset,alpha,w,DATA.scandata);
if t=false then
return;
fi;

if t=false then return; fi;

f:=alpha;i:=1;
fp:=DATA.one;
Expand Down Expand Up @@ -2869,10 +2867,12 @@ local ded,offset,pair,alpha,x,p,w;
fi;
od;
fi;
# separate 'if' check, as the `break;` only ends innermost loop
if p[alpha]=alpha then
alpha:=DATA.ct[x+offset][alpha]; # beta
if p[alpha]=alpha then
for w in DATA.ccr[x+offset] do
# AH, 9/13/18: It's R^c_{x^-1}, so -x
for w in DATA.ccr[offset-x] do
Copy link
Member

Choose a reason for hiding this comment

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

Question: I assume this is what the commit message calls "Also fixed wrong index in tracing backwards." You also wrote that this PR is purely about performance, not fixing a bug. So, I gather this means that the above bug could not have lead to incorrect results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is code to trace relators through the coset table. The wrong sign used relators starting with x, not with x^-1 (Holt Handbook, p.166, code line 11), which will not give a new result here. As consequence, the coset enumeration will take much longer to terminate (potentially might not terminate), but as it is tracing of valid relators this cannot cause wrong results.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation

if DATA.augmented then
NEWTC_ModifiedScan(DATA,alpha,w,DATA.one);
else
Expand All @@ -2889,9 +2889,11 @@ end;

NEWTC_DoCosetEnum:=function(freegens,freerels,subgens,aug,trace)
local m,offset,rels,ri,ccr,i,r,ct,A,a,w,n,DATA,p,ds,dr,
oldead,with,collapse,j,from,pp,PERCFACT,ap;
oldead,with,collapse,j,from,pp,PERCFACT,ap,ordertwo;

PERCFACT:=100;
# indicate at what change threshold display of coset Nr. should happen
PERCFACT:=ValueOption("display");
if not IsInt(PERCFACT) then PERCFACT:=100; fi;

m:=Length(freegens);
A:=List(freegens,LetterRepAssocWord);
Expand All @@ -2905,6 +2907,24 @@ local m,offset,rels,ri,ccr,i,r,ct,A,a,w,n,DATA,p,ds,dr,
ri:=Union(rels,List(rels,x->x^-1));
ri:=List(ri,LetterRepAssocWord);
SortBy(ri,Length);
A:=Concatenation([1..m],-[1..m]);

# are generators known to be of order 2?
ordertwo:=[];
for i in [1..Length(ri)] do
w:=ri[i];
if Length(w)=2 and Length(Set(w))=1 then
Unbind(ri[i]); # not needed any longer
a:=AbsInt(w[1]);
if not a in ordertwo then
Info(InfoFpGroup,1,"Generator ",a," has order 2");
AddSet(ordertwo,a);
A:=Filtered(A,x->x<>-a);
fi;
fi;
od;
ri:=Filtered(ri,x->IsBound(x));


# cyclic conjugates, sort by first letter
ccr:=List([1..2*m+1],x->[]);
Expand All @@ -2919,7 +2939,6 @@ local m,offset,rels,ri,ccr,i,r,ct,A,a,w,n,DATA,p,ds,dr,
# coset table in slightly different format: row (offset+x) is for
# generator x
ct:=List([1..offset+m],x->[0]);Unbind(ct[offset]);
A:=Concatenation([1..m],-[1..m]);

n:=1;
p:=[1];
Expand All @@ -2929,6 +2948,7 @@ local m,offset,rels,ri,ccr,i,r,ct,A,a,w,n,DATA,p,ds,dr,
subgword:=List(subgens,x->LetterRepAssocWord(UnderlyingElement(x))),
n:=n,offset:=offset,A:=A,limit:=2^23,
deductions:=[],dead:=0,defcount:=0,
ordertwo:=ordertwo,s:=[],
# a global list for the kernel scan function to return 4 variables
scandata:=[0,0,0,0]);

Expand Down Expand Up @@ -2967,6 +2987,13 @@ local m,offset,rels,ri,ccr,i,r,ct,A,a,w,n,DATA,p,ds,dr,
DATA.augmented:=false;
fi;

for a in ordertwo do
DATA.ct[offset-a]:=DATA.ct[offset+a];
if DATA.augmented then
DATA.aug[offset-a]:=DATA.aug[offset+a];
fi;
od;

if trace<>false then
with:=[0]; # generator by which a coset was defined
DATA.with:=with;
Expand Down Expand Up @@ -3022,6 +3049,7 @@ local m,offset,rels,ri,ccr,i,r,ct,A,a,w,n,DATA,p,ds,dr,

i:=1;
while i<=DATA.n do

for a in A do
if p[i]=i then
if ct[a+offset][i]=0 then
Expand Down Expand Up @@ -3521,7 +3549,7 @@ local DATA,rels,i,j,w,f,r,s,fam,new,ri,a,offset,p,rset,re,start,stack,pres,
od;
Info(InfoFpGroup,2,"Length =",Length(s));
r:=s;
if warn and Length(s)>10*Sum(rels,Length) then
if warn and Length(s)>100*Sum(rels,Length) then
warn:=false;
Error(
"Trying to eliminate all auxillary generators might cause the\n",
Expand Down
49 changes: 1 addition & 48 deletions lib/tietze.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,54 +393,7 @@ DeclareGlobalFunction("AddRelator");
## <P/>
## This time we end up with a shorter presentation.
## <P/>
## As an example of an implicit call of the function
## <Ref Func="DecodeTree"/> via the command
## <Ref Func="PresentationSubgroupMtc"/> we handle a subgroup
## of index 240 in a
## group of order 40320 given by a presentation due to
## B.&nbsp;H.&nbsp;Neumann.
## Note that we increase the level of <Ref InfoClass="InfoFpGroup"/>
## temporarily to get some additional output.
## <P/>
## <Example><![CDATA[
## gap> F3 := FreeGroup( "a", "b", "c" );;
## gap> a := F3.1;; b := F3.2;; c := F3.3;;
## gap> G := F3 / [ a^3, b^3, c^3, (a*b)^5, (a^-1*b)^5, (a*c)^4,
## > (a*c^-1)^4, a*b^-1*a*b*c^-1*a*c*a*c^-1, (b*c)^3, (b^-1*c)^4 ];;
## gap> a := G.1;; b := G.2;; c := G.3;;
## gap> H := Subgroup( G, [ a, c ] );;
## gap> SetInfoLevel( InfoFpGroup, 1 );
## gap> P := PresentationSubgroupMtc( G, H );;
## #I there are 4 generators and 144 relators of total length 4373
## #I there are 4 generators and 140 relators of total length 3898
## #I there are 4 generators and 137 relators of total length 3715
## #I there are 4 generators and 135 relators of total length 3380
## #I there are 4 generators and 134 relators of total length 3291
## #I there are 4 generators and 133 relators of total length 3212
## #I there are 3 generators and 129 relators of total length 6494
## #I there are 3 generators and 128 relators of total length 5465
## #I there are 3 generators and 127 relators of total length 3722
## #I there are 3 generators and 126 relators of total length 3048
## #I there are 2 generators and 10 relators of total length 118
## #I there are 2 generators and 5 relators of total length 38
## gap> TzGoGo( P );
## gap> TzPrintGenerators( P );
## #I 1. _x1 19 occurrences
## #I 2. _x2 19 occurrences
## gap> TzPrint( P );
## #I generators: [ _x1, _x2 ]
## #I relators:
## #I 1. 3 [ 1, 1, 1 ]
## #I 2. 3 [ 2, 2, 2 ]
## #I 3. 8 [ 1, -2, 1, -2, 1, -2, 1, -2 ]
## #I 4. 8 [ 2, 1, 2, 1, 2, 1, 2, 1 ]
## #I 5. 16 [ 2, -1, 2, 1, 2, -1, 2, 1, 2, -1, 2, 1, 2, -1, 2, 1 ]
## gap> K := FpGroupPresentation( P );
## <fp group on the generators [ _x1, _x2 ]>
## gap> SetInfoLevel( InfoFpGroup, 0 );
## gap> Size( K );
## 168
## ]]></Example>
## </Description>
## </ManSection>
## <#/GAPDoc>
Expand Down Expand Up @@ -1603,7 +1556,7 @@ DeclareAttribute("TzOptions",IsPresentation,"mutable");
## <A>P</A>.
## <Example><![CDATA[
## gap> TzPrintOptions( P );
## #I protected = 2
## #I protected = 0
## #I eliminationsLimit = 100
## #I expandLimit = 150
## #I generatorsLimit = 0
Expand Down
8 changes: 8 additions & 0 deletions tst/testbugfix/2018-09-13-MTC.tst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#MTC memory request/drop issue , Github PR#2812
# was reported in forum email
# https://mail.gap-system.org/pipermail/forum/2018/005793.html
gap> F:=FreeGroup("a","b");;
gap> rels:=ParseRelators(F,"a2,b4,(ab)11, (ab2)5,[a,bab]3,(ababaB)5");;
gap> G:=F/rels;;
gap> Size(G);
443520
Copy link
Member

@fingolfin fingolfin Sep 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Some timings on an Ubuntu server:
GAP 4.5.7: ~17.5 seconds
GAP 4.6.5: ~17.5 seconds
GAP 4.7.9: ~17.5 seconds
GAP 4.8.9: ~14 seconds
GAP master: 519 minutes
GAP with first version of this PR: ~189 seconds
GAP with revised PR: ~51 seconds

So, while this is clearly a great improvement (and thus should be merged, and backported to stable-4.10), we are still slower by a factor 13 3.5 compared to GAP 4.8 :-(. It'd be interesting to run the profiler on this to see where we spend the time now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The regression is the whole code.

There was a reported bug in the MTC that none of us was able to fix. Thus I recoded the MTC from scratch, following Holt's book. (Also, with the quiet goal of making the code better to understand and easier to expand/modify). Most of the code now is in the library, while most of the old code was in the kernel, in particular the tracing/coincidence routines (which is where the TC spends time).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for usability, I also added a small change to how cyclic subgroups are found that allows for a smaller index MTC and thus reduces the user wait time by another factor 3.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it now only takes 51 seconds; also, the computation w/o this PR terminated after 519 minutes.

And yeah, I realize that this is due to the completely new MTC implementation -- and I still think moving to your new MTC implementation was the right choice. And since it is new, I am very hopeful that we can further improve it. Besides algorithmic tweaks and fixes like in this PR (great work), a profile might reveal a few more hotspots; it might be that optimizing those (perhaps also rewriting a few more bits in C) could get us even closer to old times. Or maybe not, my point is, we haven't really tried yet to optimize this further, which makes me quite hopeful that there are opportunities for it left. So I am quite positive about this.

Copy link
Member

@dimpase dimpase Sep 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@hulpke - would you mind giving a link to the report on the original MTC bug? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dimpase

the report was a forum email (how to link, as we have no official archive?). Also it only specified "Mathieu groups are slow" and did not bother to supply the presentation.
If someone wants to submit it as a formal issue, I'll link to that.

Copy link
Member

Choose a reason for hiding this comment

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

@hulpke we do have an official archive, which can be found from the GAP website via The GAP Forum -> The GAP Forum Archive -> Archive of postings since December 2003 etc.

The message in question is https://mail.gap-system.org/pipermail/forum/2018/005793.html.

Copy link
Member

Choose a reason for hiding this comment

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

oops, clicked on "Resolve conversation", and some comments are gone - no idea how to undo, so just copying from email:

namely: from @hulpke
@dimpase
The bug in the old MTC was #302, the change of the MTC #843