Validate arguments to ListPerm, and turn it into a kernel function to make it faster
#5566
Conversation
fingolfin
added
kind: enhancement
src/permutat.cc
Outdated
| UInt lmp; // largest moved point | ||
| UInt i; // loop variable | ||
|
|
||
| lmp = DEG_PERM<T>(perm); |
There was a problem hiding this comment.
I guess from the variable name lmp that this is meant to be the largest moved point, right? If so, then this isn't correct I don't think. The degree of a permutation is the number of entries in the array storing the permutation, and this can be completely different from the largest moved point. For example if you do:
gap> x := (1, 70000);
(1,70000)
gap> y := x * x ^-1;
()
Then the degree of y is 70000 while the largest moved point is 0 (this certainly used to be the case, and the fact that y is in IsPerm4Rep seems to indicate that it still is).
There was a problem hiding this comment.
You are absolutely right. I'll add the test case and fix the PR. Thanks for carefully checking, much appreciated!
There was a problem hiding this comment.
This fix isn't complete -- lmp can still be larger than len if len==0, and cause a segfault. For example:
x:=(1,2,3,10000);
y:=(1,10000);
ListPerm(x*y);
(This won't crash immediately, but will if you keep using GAP, as we have written off into as yet unused space).
Maybe just remove the else from 1113, to make sure lmp is never larger than len?
There was a problem hiding this comment.
thanks, fixed now as well
There was a problem hiding this comment.
I've also revised the code some more, renamed lmp, etc.
Makes ListPerm about 5% slower on my system. Indeed, before this patch:
gap> pi:=(1,2,3,4);;GASMAN("collect");;
gap> for i in [1..10^7] do ListPerm(pi); od; time;
1670
After this patch:
gap> pi:=(1,2,3,4);;GASMAN("collect");;
gap> for i in [1..10^7] do ListPerm(pi); od; time;
1760
If there is concern about this, then one could simply turn ListPerm into
a kernel function, which I estimate would make it about 5 times faster,
based on this:
gap> r:=[1..4];;pi:=(1,2,3,4);;GASMAN("collect");;
gap> for i in [1..10^7] do OnTuples(r,pi); od; time;
373
fingolfin
force-pushed
the
mh/ListPerm
branch
3 times, most recently
from
81435eb
to
7496afa
Compare
The old name and comment were misleading
fingolfin
force-pushed
the
mh/ListPerm
branch
from
7496afa
to
9f958e8
Compare
|
I'm now happy for this to be merged |
|
@ChrisJefferson then please approve (and I'll wait some more before merging in case @james-d-mitchell has anything to add) |
fingolfin
added
release notes: use title
fingolfin
changed the title
ListPerm, and turn it into a kernel function to make it fasterListPerm, and turn it into a kernel function to make it faster
Resolves #4205
In that issue, @frankluebeck was concerned about performance overhead for validating the arguments. I measure it at about 5% on my laptop (going from 167 nanoseconds to 176 ns), which I personally consider fine. But to avoid any argument about that, I just rewrote it as a kernel function, after which it takes 21 ns, i.e., 8 times faster, including full argument validation.