gap icon indicating copy to clipboard operation
gap copied to clipboard

Make a safer/easier TRIM_PERM

Open ChrisJefferson opened this issue 6 years ago • 5 comments

TRIM_PERM/TrimPerm is used when a permutation's internal representation contains values bigger than it's LargestMovedPoint -- alll such values must map to themselves. In this case we can save memory by shrinking the permutation by removing all such values.

TRIM_PERM requires as an input a size to trim the permutation to. This has two problems:

  1. If this is smaller than the LargestMovedPoint, horrible things happen.
  2. If this is larger than the LargestMovedPoint, memory is wasted.

A simple method would be to just calculate the LargestMovedPoint, and trim to that. This would make a method which was easier to use, and couldn't be used to corrupt memory (except in HPC-GAP, which is a different problem).

I suggest we introduce such a method (I can't think of a good name.. SquashPerm? ShrinkPerm?), and port all uses of TrimPerm over to it.

We could in principle make a general method (ShrinkAllocation?) and implement it for all types (Perm, Trans, Plists, records,...) which would try to free any unused memory. This method already exists for plists and strings, and is called ShrinkAllocationPlist and ShrinkAllocationString.

ChrisJefferson avatar Jul 15 '19 18:07 ChrisJefferson

One option for an improvement would be, whenever you trim a perm, to also trim its stored inverse (if it has one) to the same length, rather than just throwing it away, which is what #3575 implements.

wilfwilson avatar Jul 16 '19 09:07 wilfwilson

There is another issue with TRIM_PERM: resizing and retyping of permutations is a potential source of issues for HPC-GAP (and in the future, "Julia-GAP" / "GAPJulia"). It's not good if a supposedly read-only / immutable / constant object which is used in multiple threads changes its representation. It would be somewhat less of a problem if we changed the way trimming is implemented: first, create a new permutation of the desired type and size; fill it with the new data; then perform a master pointer swap on them, which can essentially be done atomic (I think, though the devil is in the details). Even better would be to just not do this at all... Perhaps @rbehrends has some additional thoughts on this.

fingolfin avatar Jul 17 '19 07:07 fingolfin

Hmm, off the top of my head I don't see a better solution for HPC-GAP than the one you suggested. The problem is that permutations are public, so we can't special case the situation where only the current thread is using one.

rbehrends avatar Jul 17 '19 08:07 rbehrends

Swapping master pointers doesn't help, as once a thread has looked at a permutation and found it's a perm4, it will continue treating it like one, something like:

Thread 1: Check TNUM of `p`, find out it's a perm4.
Thread 2: Do a master pointer swap  on `p` and change it to a perm2.
Thread 1: Start reading the insides of `p`, as if it were a perm4 (which it is not, any more).

ChrisJefferson avatar Jul 17 '19 09:07 ChrisJefferson

Related: in an ideal world also TRIM_TRANS and TRIM_PPERM would be "replaced" (resp. their high level wrappers TrimTransformation resp. TrimPartialPerm).

In packages these trim functions are only used by orb, for the hash functions there. This could be avoided by adding TrimmedPerm etc. functions which create a new object -- that'd be inefficient of course, but I think this corner case is rare, and it's not like the same element would get hashed that many times.

Same for the SparseIntKey in the library.

Also for the ImagesRepresentative, "perm group hom",FamSourceEqFamElm method using TRIM_PERM

This leaves TrimStabChain which calls TRIM_PERM... This one is delicate because the elements being modified are referenced in multiple places...

fingolfin avatar Sep 18 '25 15:09 fingolfin