Caching Types

[hulpke]

The following observation is the result of several hours debugging (with PR #2387) why a group thought it was nilpotent, though this is not true.

GroupWithGenerators caches group types in the elements family (for speed reason -- don't recreate the same type). However it seems that once assigned to a group these types can change and accumulate further properties that are not true in general. (This is the best explanation I could come up and avoiding type caching made the problem go away.)
Concretely, what I suspect is that at the first occurrence of creating and then changing a type, the type gets changed, as it is used only once. W/o #2387 this change is something harmless done by an immediate method (such as being not empty)

Is this a correct observation, i.e. should we avoid caching types (which is currently done for other objects as well, e.g. polynomials)? If so is this intended?

If it is intended there are other places where types are cached (e.g. polynomials) and require similar care -- I can do library changes if needed.

[fingolfin]

Hmm, normally, setting filters on an object should cause a fresh type object to be created (which I believe is the main reason immediate methods cause such a performance overhead). I.e. type objects should be effectively immutable, we worked hard to achieve that in order to help HPC-GAP, where they are indeed marked as "read only" objects. I also just added MakeImmutable(type); to the end of NEW_TYPE, and GAP seems to start fine with that.

Also, in e.g. SetFilterObj( obj, filter ), we do this:

      type:= TYPE_OBJ( obj );
      newtype:= Subtype2( type, filter );
      SET_TYPE_POSOBJ( obj, newtype );

What you describe should thus not happen, or rather indicate a bug in the caching logic. E.g. in PR #2387, if one were to set IsNilpotentGroup before creating the cached type, then of course that property would (incorrectly) propagate to all groups using that cached type. But that then would be a bug in the code creating the cache, and one that can be fixed.

But of course that's just in theory, of course bugs can lurk anywhere ;-).

Can you give an example for reproducing this?

[stevelinton]

Agreed. Once a type is created, it should never change. I don't think making it immutable will make any difference. It might be more interesting to make the flags list object of a type immutable, but most changes to it probably happen in the kernel, so they will most likely not check.

I'd look at the new WITH_IMPS_FLAGS machinery. If a flag list is being modified there instead of copied under some circumstances (or the old list is being modified instead of the copy or whatever) then that might cause something like this.

[hulpke]

Thank your for looking into this. I added caching again to try to reproduce the error (in a slightly different form) and the error did not appear. However in doing this I noted (line 4359 in grp.gi) that GroupWithGenerators for the empty list re-uses defaultFinitelyGeneratedGroupWithOneType but sets further flags. That could have been an alternative culprit. I have reintroduced caching in #2387 .
Apologies for causing you work!

[I think I can close this now?]

[hulpke]

Closing at the issue seems to be a non-issue.