Improve performance of Info, and add BindConstant #1770

ChrisJefferson · 2017-10-09T15:17:24Z

This patch rearranges how we implement Info, to make it more efficient.

Just to give a feeling for the times, I wrote a test which compares an if statement against anInfo.

f := function() local i; for i in [1..10000000] do if x = 2 then Print("!!"); fi; od; end;;

g := function() local i; for i in [1..10000000] do Info(InfoDebug, 2, "!!"); od; end;;

f: 124ms

g in 4.8: 3086ms (24x slower)
g in master: 980ms (8x slower)
g with this patch: 217ms (1.75x slower)

To go any faster, I think we would have to completely tear out the current system, remove the extensibility, and possibly introduce a TNUM for Info objects, so we could avoid any error checking.

EDIT: This also includes a new little function, BindConstant. This is like BindGlobal, but for Constant globals -- it just makes a quick easy way to declare constants. I experiment (I think successfully) with using it to access positional objects efficiently.

codecov · 2017-10-09T16:59:37Z

Codecov Report

Merging #1770 into master will increase coverage by 12.68%.
The diff coverage is 67.64%.

@@             Coverage Diff             @@
##           master    #1770       +/-   ##
===========================================
+ Coverage   50.42%   63.11%   +12.68%     
===========================================
  Files         449      967      +518     
  Lines      234267   292951    +58684     
  Branches    10422    12976     +2554     
===========================================
+ Hits       118120   184883    +66763     
+ Misses     113370   105242     -8128     
- Partials     2777     2826       +49

Impacted Files	Coverage Δ
lib/info.gi	`60.62% <64.28%> (+15.51%)`	⬆️
src/intrprtr.c	`71.24% <83.33%> (+12.33%)`	⬆️
src/funcs.c	`74.89% <0%> (-15.45%)`	⬇️
src/intobj.h	`81.48% <0%> (-9.43%)`	⬇️
src/dteval.c	`2.79% <0%> (-0.29%)`	⬇️
src/dt.c	`1.61% <0%> (-0.26%)`	⬇️
hpcgap/pkg/gapdoc/lib/GAPDoc2LaTeX.gi	`1.5% <0%> (-0.12%)`	⬇️
hpcgap/pkg/gapdoc/lib/GAPDoc2Text.gi	`0.54% <0%> (-0.05%)`	⬇️
src/pperm.h	`100% <0%> (ø)`	⬆️
lib/helpview.gi	`0% <0%> (ø)`	⬆️
... and 839 more

stevelinton · 2017-10-10T15:39:55Z

Maybe change the title to mention BindConstant. Otherwise looks great to me.
I wrote the original Info code long ago as a placeholder and then forgot about it.

fingolfin

Nice! A performance improvement, and the new code is (IMHO) simpler, too. I still have some remarks.

Also, this PR also seems to merge the info code for HPC-GAP back into the regular GAP library, but neither the PR nor the relevant commit mention this -- IMHO, both should.

fingolfin · 2017-10-12T10:23:13Z

lib/info.gi

-    if IsBound(HPCGAP) then
-        ShareInternalObj(InfoData);
-    fi;
+# Give constants for the members of info class


This is a very strange sentence. Perhaps "Define constants indicating the positions of the members of the info class" ?

fingolfin · 2017-10-12T10:23:35Z

lib/info.gi

 ##
-##  these are all lists, and the first two should have the same length, 
-##  which defines the number of Info classes that exist
+##  ClassNum is placed at the end so the length of the list never changes


I don't understand this sentence.

I'll explain it better. I don't want to put optional members at the end, because then the list of the length can change when those optional members become assigned, which doesn't work for IsAtomicPositionalObjects, which use fixed length atomic lists (that comment really is too short).

fingolfin · 2017-10-12T10:23:58Z

lib/info.gi

+BIND_CONSTANT("INFODATA_OUTPUT", 4);
+BIND_CONSTANT("INFODATA_NUM", 5);
+
+# A list of all created Info Classes


Classes -> classes

Oh, I see lots of other places in this file write "Info Class" or "Info Classes". Ah well, I guess you are being consistent here then -- still seems strange, but ... shrug

fingolfin · 2017-10-12T10:24:28Z

lib/info.gi

+INFO_CLASSES := [];
+
+# The most recently called Info statement
+InfoData := rec();


How can a record be "the most recently called Info statement"?

I actually think that we can get rid of InfoData but that can be done in a later PR (I can take care of it, too).

In the meantime, perhaps say something like this:

The level and (first) selector of the most recently executed Info statement is stored in InfoData

I will improve the comment. Note that InfoData is used by GAPDoc. I tried hard in this commit to not break any way someone might have extended the Info, in case anyone has.

fingolfin · 2017-10-12T10:31:53Z

lib/info.gi

-        pos := Position(InfoData.ClassNames,name);
-        if pos <> fail then
-            return InfoData.InfoClass(pos);
+    atomic readwrite INFO_CLASSES do


So... are we now OK with adding atomic back to the library, outside of if IsBound(HPCGAP) code paths? So far, there are only two atomic in the non-HPC GAP library, both in init.g.

I thought we were, now that the parser removes them in GAP. However, we should make sure we all agree about that!

Well, to make sure we all agree, "somebody" has to bring it up on e.g. the mailing list...

Ahh, whatever, let's just merge this in. It's deep in internal code, and one can understand the code by just ignoring the atomic here. If somebody truly minds, I'll wait for them to bring it up on the mailing list or elsewhere.

fingolfin · 2017-10-12T10:35:23Z

src/intrprtr.c

-    }
-    else {
-        return CALL_2ARGS(InfoDecision, selectors, level);
+#endif


Perhaps just use ELM_LIST here, to replace five lines by one?

Because ELM_LIST doesn't work on positional objects, or atomic positional objects.

If we make it work, then given a positional object X[3] starts working at the GAP level, rather than the X![3] we expect people to use.

However, some kind of unified access would be nice, as this code is fairly horrible and I wouldn't want to start writing this regularily.

OK, that makes sense.

fingolfin · 2017-10-12T10:36:50Z

src/intrprtr.c

+        if (IS_INTOBJ(index) && IS_INTOBJ(level)) {
+            Obj res;
+            LT_INTOBJS(res, index, level);
+            if (res == True) {


That seems needlessly complicated -- why not replace the above three lines by if (index < level) ?

I agree it's complicated, but I couldn't find a shorter way of comparing integers "properly". I could just use the fact that I know that < works for a pair of INTOBJs.

fingolfin · 2017-10-12T12:32:50Z

src/intrprtr.c

 static Obj IsInfoClassListRep;

 Obj InfoCheckLevel(Obj selectors, Obj level)
 {
-    if (IS_POS_INTOBJ(level) &&
-        CALL_1ARGS(IsInfoClassListRep, selectors) == True) {
+    // Fast-pass the most common failing case


I think elsewhere we call(ed) this a "fast-path", not a "fast-pass"

fingolfin · 2017-10-12T12:41:05Z

src/intrprtr.c

-        CALL_1ARGS(IsInfoClassListRep, selectors) == True) {
+    // Fast-pass the most common failing case
+    // The fast-pass never returns True because the GAP function
+    // InfoDecision sets a number of variables as well as returning True


This explanation feels a bit backwards to me (but that may just me being dense). I'd suggest something like this:

The fast-path only deals with the case were False is returned, as in the
True case, InfoData.LastClass and InfoData.LastLevel need to be set.

But actually, I think we can get rid of this exception, by getting rid of InfoData. In the meantime, though, of course it makes sense to document this exception.

ChrisJefferson · 2017-10-13T09:39:16Z

This is now improved, fixed all comments, and added tests, except for the slightly horrible "ifdef HPCGAP" checking inside lists bit, where we should indeed come up with something better (or I should find out how I'm supposed to read members of positional objects)

fingolfin · 2017-10-19T10:05:38Z

lib/info.gi

-        pos := Position(InfoData.ClassNames,name);
-        if pos <> fail then
-            return InfoData.InfoClass(pos);
+    atomic readwrite INFO_CLASSES do


Well, to make sure we all agree, "somebody" has to bring it up on e.g. the mailing list...

fingolfin · 2017-10-19T10:19:40Z

lib/global.g

+    MAKE_CONSTANT_GLOBAL(name);
+    return val;
+end;
+MAKE_READ_ONLY_GLOBAL("BIND_CONSTANT");


Nitpick: Why not use BIND_GLOBAL for defining BIND_CONSTANT, i.e. BIND_GLOBAL("BIND_CONSTANT", ...) ?

fingolfin · 2017-10-19T10:21:02Z

lib/global.gd

-##  <A>val</A>, provided it is writable, and makes it read-only.
+##  <Ref Func="BindGlobal"/> and <Ref Func="BindConstant"/> set the global
+##  variable named by the string <A>name</A> to the value
+##  <A>val</A>, provided it is writable. <Ref Func="BindGlobal"/> makes


The "it" in "it is writable" is ambiguous. Perhaps "provided that variable is writable", or even "provided that variables is unbound, or bound and writable".

[Aha, but I just realize the ambiguous text was there before. sigh]

fingolfin · 2017-10-19T10:37:14Z

src/intrprtr.c

-    }
-    else {
-        return CALL_2ARGS(InfoDecision, selectors, level);
+#endif


OK, that makes sense.

fingolfin · 2017-10-19T10:37:53Z

src/intrprtr.c

-    else {
-        return CALL_2ARGS(InfoDecision, selectors, level);
+#endif
+        if (IS_INTOBJ(index) && IS_INTOBJ(level)) {


You could use ARE_INTOBJS here. But the above is of course fine, too.

ChrisJefferson · 2017-10-19T15:47:32Z

This should wait until after #1789

stevelinton · 2017-10-27T13:56:17Z

Looks like this needs a rebase and a trivial conflict resolved. Then I'd be happy to merge it -- trying to get as many performance patches in as is safe.

fingolfin · 2017-10-30T15:42:50Z

lib/global.gd

+##  <Ref Func="BindGlobal"/> and <Ref Func="BindConstant"/> set the global
+##  variable named by the string <A>name</A> to the value
+##  <A>val</A>, provided it is writable. <Ref Func="BindGlobal"/> makes
+##  the resulting variable read-only, which <Ref Func="BindConstant"/> makes


Just noticed: "which" -> "while"

stevelinton · 2017-11-01T13:52:31Z

I'd be happy to see this rebased and merged. It's another useful performance improvement and makes code overall much nicer.

This patch moves IsInfoClassRep information out of a global structure into the class, to improve performance. Also add a special case at the Kernel level to optimise the most common situation. This reduces the speed gap between Info and an equivalent if statement from 10x to ~1.5x Also remove the need for a seperate HPC-GAP implementation

fingolfin · 2017-11-01T15:28:50Z

Can be merged from my POV once tests passed

ChrisJefferson · 2017-11-01T20:14:02Z

Now passing, and the bindconstant removed.

ChrisJefferson force-pushed the info-improve branch from 6e98b0b to a298dc4 Compare October 10, 2017 12:15

ChrisJefferson changed the title ~~Info improve~~ Info improve and add BindConstant Oct 11, 2017

fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Oct 12, 2017

fingolfin added this to the GAP 4.9.0 milestone Oct 12, 2017

fingolfin requested changes Oct 12, 2017

View reviewed changes

ChrisJefferson force-pushed the info-improve branch 2 times, most recently from bf8b49e to 7390f5a Compare October 13, 2017 08:12

ChrisJefferson force-pushed the info-improve branch from 7390f5a to 78599de Compare October 13, 2017 09:45

fingolfin mentioned this pull request Oct 19, 2017

Compiler emits call to InfoDecision but symbol is defined static in intrprtr.c #1781

Closed

fingolfin approved these changes Oct 19, 2017

View reviewed changes

fingolfin reviewed Oct 30, 2017

View reviewed changes

fingolfin mentioned this pull request Oct 30, 2017

Add BindConstant, BIND_CONSTANT and use them #1827

Merged

ChrisJefferson force-pushed the info-improve branch 3 times, most recently from 750e32b to cbdf9d3 Compare October 31, 2017 20:03

ChrisJefferson force-pushed the info-improve branch from cbdf9d3 to 99dd8b3 Compare November 1, 2017 14:33

fingolfin approved these changes Nov 1, 2017

View reviewed changes

stevelinton merged commit f4de824 into gap-system:master Nov 1, 2017

olexandr-konovalov changed the title ~~Info improve and add BindConstant~~ Improve performance of Info, and add BindConstant Jan 29, 2018

olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 29, 2018

ChrisJefferson deleted the info-improve branch February 19, 2018 15:53

Improve performance of Info, and add BindConstant #1770

Improve performance of Info, and add BindConstant #1770

Conversation

ChrisJefferson commented Oct 9, 2017 • edited Loading

codecov bot commented Oct 9, 2017 • edited Loading

Codecov Report

stevelinton commented Oct 10, 2017

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisJefferson commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisJefferson commented Oct 19, 2017

stevelinton commented Oct 27, 2017

Choose a reason for hiding this comment

stevelinton commented Nov 1, 2017

fingolfin commented Nov 1, 2017

ChrisJefferson commented Nov 1, 2017

ChrisJefferson commented Oct 9, 2017 •

edited

Loading

codecov bot commented Oct 9, 2017 •

edited

Loading