Speed up IsRegularPGroup
#5797
Speed up IsRegularPGroup
#5797
Conversation
fingolfin
added
topic: performance
fingolfin
force-pushed
the
mh/tune-IsPowerfulPGroup
branch
2 times, most recently
from
833ef66
to
aba3e9d
Compare
|
@jackschmidt perhaps you'd be willing to give this patch a glance over? |
fingolfin
force-pushed
the
mh/tune-IsPowerfulPGroup
branch
from
aba3e9d
to
eb74620
Compare
There was a problem hiding this comment.
Thank you! These changes look good to me. I have not had a chance to time check them, or to try to find pathological examples, but the only line that potentially adds more work is the IsOne(g*h), and I hope that is negligible compared to everything else. All other changes should strictly reduce the amount of work done if the slow path is taken (and don't do anything if one of the earlier quick checks applies).
|
|
||
| if not IsPGroup(G) then | ||
| return false; | ||
| fi; | ||
|
|
||
| p:=PrimePGroup(G); | ||
| if p = 2 then | ||
| # see [Hup67, Satz 10.3 a)] | ||
| # see [Hup67, Satz III.10.3 a)] |
There was a problem hiding this comment.
Good. The previous citation was incomplete. This is correct
| # we just check the direct definition, with a minor twist to avoid | ||
| # calling Agemo | ||
| g := ap_bp / ab_p; | ||
| h := Comm(a,b)^p; |
There was a problem hiding this comment.
Good. All the caching is correct and should avoid repeated element calculations
| # calling Agemo | ||
| g := ap_bp / ab_p; | ||
| h := Comm(a,b)^p; | ||
| if g = h or IsOne(g*h) then continue; fi; |
There was a problem hiding this comment.
Good. This new check is most advantageous for small p, but should always be cheap (g=h always cheap, g*h should not be expensive). For small p, I suspect it completely avoids the subgroup check in many cases
There was a problem hiding this comment.
Thank you very, very much for you feedback @jackschmidt and sorry for taking so long to get back to you.
Regarding this point, indeed g=h is fast and indeed for small IsOne(g*h) is absolutely worth it, based on extensive tests with groups of order
lib/grp.gi
Outdated
| H := DerivedSubgroup(H); | ||
| if not (ap_bp / ab_p) in Agemo(H, p) then | ||
| H := NormalClosure(H, [h]); # faster than Agemo(DerivedSubgroup(H), p); | ||
| if not g in H then |
There was a problem hiding this comment.
Good. This new H should be much faster to compute, and if g is not in H, then G is surely not regular, but if G is regular, then this H is the same as the more expensive Agemo subgroup, and the check will always pass.
There was a problem hiding this comment.
I've now added a comment explaining what's going on, I hope this makes it much clearer. Apologies for not doing that before, I really should have :-(
fingolfin
force-pushed
the
mh/tune-IsPowerfulPGroup
branch
from
eb74620
to
afbffaf
Compare
The speed up is achieved by carefully caching some more
intermediate results. Also avoid an Agemo call, and drop a size
check that I thought was beneficial, but in benchmarking harder
examples turned out to be not helpful.
Some timeing on my M1 MacBook Pro. Before this patch:
gap> IsRegularPGroup(DirectProduct(SmallGroup(3^5,22),SmallGroup(3^5,22)));; time;
27609
gap> IsRegularPGroup(DirectProduct(SmallGroup(3^5,22),SmallGroup(3^5,39)));; time;
77479
After this patch:
gap> IsRegularPGroup(DirectProduct(SmallGroup(3^5,22),SmallGroup(3^5,22)));; time;
1488
gap> IsRegularPGroup(DirectProduct(SmallGroup(3^5,22),SmallGroup(3^5,39)));; time;
3957
Also fix some references to Huppert's book.
fingolfin
force-pushed
the
mh/tune-IsPowerfulPGroup
branch
from
afbffaf
to
11a0bc4
Compare
The speed up is achieved by carefully caching some more
intermediate results. Also avoid an Agemo call, and drop a size
check that I thought was beneficial, but in benchmarking harder
examples turned out to be not helpful.
Some timeing on my M1 MacBook Pro. Before this patch:
After this patch:
Also fix some references to Huppert's book.