Rewrite method selection in C

[fingolfin]

This is not about performance (the difference seems to be negligible; the method cache is doing a darn good job, it seems). Instead, the main motivation for this is simplification and reduction of code duplication: less than 100 lines of documented C code replace > 1500 lines of GAP code, as well as > 5600 lines of C code generated from that GAP code.

To verify that the new code is correct, I used it in parallel to the old code, adding in a check to verify that old and new method lookup code agree (the commit with this state is in this PR, so one can easily reproduce this). This passed the testinstall test suite (after revealing lots of bugs in my initial code, that is ;-).

The main drawback of this effort is that yet a bit more of low-level GAP code is replaced by GAP code, which might annoy people like @markuspf who are interested in reimplementing the GAP kernel in languages other than C. However, I think this PR actually does not add much to the burden, as the new C function still is pretty straight forward to rewrite in other languages, which one has to do with the rest of the method selection code anyway.

[codecov]

Codecov Report

Merging #2308 into master will increase coverage by 0.15%.
The diff coverage is 81.65%.

@@            Coverage Diff             @@
##           master    #2308      +/-   ##
==========================================
+ Coverage   72.33%   72.49%   +0.15%     
==========================================
  Files         479      478       -1     
  Lines      250960   247780    -3180     
==========================================
- Hits       181538   179617    -1921     
+ Misses      69422    68163    -1259

Impacted Files	Coverage Δ
src/gapstate.h	71.42% <ø> (ø)	⬆️
lib/methsel2.g	43.93% <ø> (ø)	⬆️
src/opers.c	92.58% <79.56%> (+0.43%)	⬆️
lib/methsel.g	62.5% <93.75%> (+59.3%)	⬆️
hpcgap/lib/hpc/queue.g	66.4% <0%> (-3.2%)	⬇️
src/hpc/traverse.c	95.75% <0%> (-0.59%)	⬇️
src/io.c	72.42% <0%> (-0.38%)	⬇️
src/compiler.c	87.03% <0%> (-0.29%)	⬇️
hpcgap/lib/hpc/stdtasks.g	38.61% <0%> (-0.26%)	⬇️
src/gap.c	80.29% <0%> (-0.09%)	⬇️
... and 4 more

[ChrisJefferson]

I'm mostly happy with this. My only concern (as with so much of GAP), is lack of testing (which didn't exist before!)

For example, in the tracing we used to print #I Trying Next: sometimes, which has now been removed, which means the tracing output will have changed. I can't figure out how to make GAP print that message in master, to then check the new output is reasonable... do you know?

[stevelinton]

Just out of curiosity (and to address the "more C code" objection) how much performance do we lose if we just stop bothering to compile these functions (thereby getting rid of the 5600 lines of C) and then look at compacting the GAP code. If the method cache really is doing all the heavy lifting, then it shouldn't make much difference.

That said, the C code is commented and readable and much more concise than the current GAP code, so I'm quite happy with it.

[stevelinton]

Maybe add a comment to explain the point of the ALWAYS_INLINE and that you expect it to be called with
some of the parameters compile-time constants.

[fingolfin]

Will do.

[fingolfin]

@ChrisJefferson regarding testing, I actually started to add testing for a lot of these things in PR #2296, including (old-style) profiling and tracing -- admittedly for both only tiny infant versions, but it's a start, and I hope more can and will be added.

As to "Trying next" being lost" oops, that's indeed a mistake, I did not notice NEXT_VMETHOD_PRINT_INFO. Will address this in an update to this PR, and try to see if I can add a test for this specifically (but I'd rather wait for PR #2296 to be merged first).

[fingolfin]

@stevelinton what do you mean by "compacting the GAP code? One thing would be to merge METHOD_0ARGS with NEXT_METHOD_0ARGS etc. (that's very straight forward). One step further then is to also merge those two with VMETHOD_0ARGS with NEXT_VMETHOD_0ARGS; then one step further is to merge them with their constructor variants. By that time, we have reduce the number of these methods by a factor of 8 already. Is that what you have in mind

Finally, one could try to merge the different arity methods together, but that sounds more complicated, so perhaps I'd stop before that.

I can look into doing that, I guess. Well, the first step would be to check how much of an impact it has to not compile methsel1.g. Anybody volunteering to run some tests with that? Iit's easy -- just add a single space to it, then GAP will use it instead of the compiled version; now run some benchmarks.

[stevelinton]

@fingolfin, I was thinking fairly generally along those lines. If we believe that code is not performance critical, then we can dramatically reduce the repetition. I agree about the first experiment to try.

[fingolfin]

Quick test, with methsel1.g compiled:

$ time (echo "QUIT;" | ./gap -r -b -A)
gap>
real	0m0.776s
user	0m0.744s
sys	0m0.028s

With it not compiled:

$ time (echo "QUIT;" | ./gap -r -b -A)
#W Static module lib/methsel1.g has CRC mismatch, ignoring
gap>
real	0m0.856s
user	0m0.828s
sys	0m0.020s

Repeating this several times gives fairly consistent results. So not compiling this code slows down startup noticeably.

On the other hand, that may be due to the method cache not yet being "warmed up". Running tst/testinstall.g shows less of a difference, but still: with compiled code it's about 75 seconds, without it about 80 seconds.
However, timings for testinstall fluctuates quite a lot for me, so perhaps a better benchmark is needed.

[ChrisJefferson]

Just one comment with regards Trying Next, depending on what is coming out of tracing, I don't think it's required to keep exactly the same tracing messages -- but we should (obviously) check they are of equivalent (or better) quality.

[fingolfin]

Updated to (hopefully) address all comments by @ChrisJefferson and @stevelinton

[stevelinton]

@fingolfin that's more than enough evidence for me to justify replacing 1500 lines of GAP with 100 lines of C.

[fingolfin]

@ChrisJefferson @stevelinton any other remarks? otherwise, can we merge this?