Reduce immutable/mutable diffs (UPDATE: now ready for merge)

[fingolfin]

UPDATE: I believe this could be merged now. Also, I submitted issue #2325 for the general (im)mutability plan.

This PR builds extends PR #1570, and again is motivated by PR #1563: It unifies some code to reduce difference between mutable and immutable cases. The number of word occurrences of IMMUTABLE, as counted by git grep -w IMMUTABLE src | wc -l goes slightly down from 1328 to 565. But once one runs the script below, it goes down to 309, i.e. a quarter of what we started with.

This now makes it much easier to decide how to deal with the rest. Some immediate observations on those:

• adding a NEW_IMM_PLIST function (possibly with a different name) would take care of quite a few of those occurences (and this function could immediately set the new OBJ_FLAG_IMMUTABLE)
• a lot of code deals with creating a new object with mutability identical to another object; could have a function for that, too
• but in a few cases, the mutability depends on a more complex logic... also, there are a bunch of RetypeBag calls to change immutability... it might be useful to have a faster variant of MakeImmutable, which is only useful for builtin lists and record, and directly modifies the TNUM (now) resp. object flag (with PR Immutable Object Flag (WIP) #1563)
• for HPC-GAP, MakeBagTypePublic cannot be used to automatically migrate immutable list and record bags to the public region. No big deal, though (and I actually like that we can thus remove some logic from RetypeBag)
• IsMutableObjFuncs likely can and should be removed once we get rid of the IMMUTABLE bit in the TNUM

Here is the script I mentioned above:

files:=[];
for dir in [ "src", "src/hpc" ] do
    dir:=Directory(dir);
    fs:=DirectoryContents(dir);
    fs:=Filtered(fs, f->EndsWith(f,".c"));
    Append(files, List(fs, f -> Filename(dir,f)));
od;


for f in files do
    Print("Processing ", f, "\n");
    data:=StringFile(f);
    lines:=SplitString(data,"\n");
    i := 2;
    while i <= Length(lines) do
        if PositionSublist(lines[i], "IMMUTABLE") = fail then
            i := i + 1;
            continue;
        fi;
        str := ReplacedString(lines[i], "+IMMUTABLE", "          ");
        if str = lines[i-1] then
            Remove(lines, i);
        fi;
        i := i + 1;
    od;
    out:=JoinStringsWithSeparator(lines, "\n");
    Add(out,'\n');
    FileString(f,out);
od;

[fingolfin]

Just added NEW_IMM_PLIST (name subject to change), now we are down to 262 occurrences.

[codecov]

Codecov Report

Merging #1571 into master will increase coverage by <.01%.
The diff coverage is 92.63%.

@@            Coverage Diff             @@
##           master    #1571      +/-   ##
==========================================
+ Coverage   73.21%   73.21%   +<.01%     
==========================================
  Files         482      482              
  Lines      246422   246379      -43     
==========================================
- Hits       180411   180395      -16     
+ Misses      66011    65984      -27

Impacted Files	Coverage Δ
src/vecgf2.c	72.05% <ø> (ø)	⬆️
src/lists.c	66.55% <ø> (-0.08%)	⬇️
src/listfunc.c	81.66% <ø> (ø)	⬆️
src/records.c	68.26% <ø> (ø)	⬆️
src/vec8bit.c	83.29% <ø> (ø)	⬆️
src/objset.c	81.69% <ø> (-0.17%)	⬇️
src/stringobj.c	83.35% <ø> (+0.36%)	⬆️
src/objects.h	100% <100%> (ø)	⬆️
src/range.c	90.48% <100%> (+0.22%)	⬆️
src/lists.h	100% <100%> (ø)	⬆️
... and 4 more

[fingolfin]

Function IS_MUTABLE_PLAIN_OBJ was meant to be kind of an optimization, but of course it's mostly redundant with the changes to IS_MUTABLE_OBJ in PR #1563.

[fingolfin]

Perhaps NEW_IMM_PLIST should be called NewImmutablePlist, and we also provide a NewMutablePlist?

[fingolfin]

I should probably point out that the changes in this PR are not that attractive w/o PR #1563, as I essentially replace dispatch via our TNUM based tables by another layer of if checks, i.e. a tiny (but possibly measurable) slowdown. This only will amortize itself (if at all!) in conjunction with PR #1563. Note that I am not really concerned about the changes for types; but for e.g. AssPRec, it might be noticeable in code which accesses precs again and again in a tight loop...

[fingolfin]

@markuspf perhaps you want to combine the two PRs? I am not going to work more on this particular PR in the near future, but perhaps you are interested in picking it up -- I see you already made some of the changes I mentioned above in your PR?

(Also, several of the commits in this PR probably could be squashed together; I just split them up like that in order to track down the crashes caused by me overlooking that T_COMOBJ is also handled by AssPRec)

[fingolfin]

I think having something like this would be helpful:

static inline Obj NEW_PLIST_WITH_GIVEN_MUTABILITY(UInt type, Int plen, Int mut)
{
  GAP_ASSERT(type & IMMUTABLE == 0);
  if (!mut)
    type |= IMMUTABLE;
  return NEW_PLIST(type, plen);
}

This could then be used to convert tons of places containing code like this:

img = NEW_PLIST( IS_MUTABLE_OBJ(pair) ? T_PLIST : T_PLIST+IMMUTABLE, 2 );

In order to introduce the (im)mutable object flag @markuspf is working on, now only that function has to be changed, instead of all the places calling it.

Of course a better name would be nice... ;-). And NEW_IMM_PLIST could then either be removed again, or re-implemented in terms of that new function.

[fingolfin]

Another potentially useful function, which could be used to gradually migrate our codebase:

void MakeImmutablePlist(Obj plist)
{
  GAP_ASSERT(IS_PLIST(plist));
#ifdef IMMUTABLE
  const UInt tnum = TNUM_OBJ(plist);
  RetypeBag( plist, tnum | IMMUTABLE );
#else // (im)mutable object flag
  SET_OBJ_FLAG(bag, OBJ_FLAG_IMMUTABLE);  
  #ifdef HPCGAP
  MakeBagPublic(plist);
  #endif
#endif
}

[markuspf]

Just as a notification: I have combined this PR and #1563, but bumped into a few bugs and problems along the way (for example #1632 and #1637), so progress is slow.
More updates on #1563 (or as a new PR) soon.

[ChrisJefferson]

I've lost track of what's up with this PR, and related things.

If this one is the best we've got, and we could rebase it, I think it's an improvement.

[fingolfin]

I just rebased this. However, I am not so sure it's useful to merge this in isolation, at least right now. The main goal (as you know) was to serve as foundation for a bigger change which gets rid of the IMMUTABLE bit in TNUMs in favor of an object flag. Without those changes, this PR contains some changes which are active de-optimizations, which makes them (IMHO) harder to justify.

So I'd rather delay this until after 4.9, and then perhaps pick up work on the whole IMMUTABLE business. So far, what I feel is missing is a more concrete vision as to how to achieve the transition (incrementally or not).

One approach is to start by adding the object flag in addition to the TNUM bit, and then add checks to verify they are in sync. @markuspf did that with the now closed PR #1563, and I think run into some problems (and fixed some, and some remain for now). So we also need to work on those issues (perhaps starting by listing them somewhere).

[markuspf]

This is also still on my radar, but work had stalled, I think I would need to take a fresh look at it.

On the upside the work on #1563 shook out the problems with InstallValue and SwapMasterPointer...

[markuspf]

Just to add, I think introducing the mutability object flag is quite feasible: the biggest stalling point was making a decision as to whether the flag should reflect mutability or immutability.

[fingolfin]

I changed my mind and would like to merge this now anyway -- I rebased it and improved it, though. Of course @ChrisJefferson already approved this ages ago, but I believe it would be sensible if he or somebody else had another brief look at this.

This was for an older, different version of this PR.