Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make a safer/easier TRIM_PERM #3576

Open
ChrisJefferson opened this issue Jul 15, 2019 · 4 comments
Open

Make a safer/easier TRIM_PERM #3576

ChrisJefferson opened this issue Jul 15, 2019 · 4 comments
Labels
kind: discussion discussions, questions, requests for comments, and so on kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel

Comments

@ChrisJefferson
Copy link
Contributor

ChrisJefferson commented Jul 15, 2019

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.

@wilfwilson
Copy link
Member

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 wilfwilson added kind: discussion discussions, questions, requests for comments, and so on kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel labels Jul 16, 2019
@fingolfin
Copy link
Member

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.

@rbehrends
Copy link
Contributor

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.

@ChrisJefferson
Copy link
Contributor Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: discussion discussions, questions, requests for comments, and so on kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel
Projects
None yet
Development

No branches or pull requests

4 participants