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

kernel: TmpPPerm is not a valid T_PPERM4 object, causes issues with Julia GC #2493

Closed
fingolfin opened this issue May 25, 2018 · 2 comments
Closed

Comments

@fingolfin
Copy link
Member

The pperm code uses a T_PPERM4 bag stored as TmpPPerm for scratch storage (and similarly in trans.c we have TmpTrans). For this, it writes integers into the whole bag area -- including the first two slots, which are supposed to be object/bag pointers. This is fine with GASMAN, but if one relies on precise scanning (i.e. everything the mark functions to be an object really is an object), one runs into a problem. In GAP, on can simulate this by #defining DEBUG_GASMAN_MARKING (@ChrisJefferson perhaps we should just enable this with --enable-debug?)

While @rbehrends work around that in Julia GC by not using precise scanning, this causes a performance overhead (although today he said this is now minimal). Still, it'd be nice to get rid of this. Several solutions come to mind:

  1. change the pperm code to use ADDR_PPERM4 instead of ADDR_OBJ to access the temp storage (similar for trans code)
  2. instead of (ab)using a T_PPERM4 / T_TRANS4 for temp storage, use some bag type which uses MarkNoSubBags as marking function, such as T_STRING
  3. introduce a T_SCRATCH tnum for purposes like this

Note that the permutation code also uses temp storage, but it uses ADDR_PERM2 resp. ADDR_PERM4 to access it, and thus does not override the first bag slot (which now is also an object ref, contrary to the documentation at the top of the file; see issue #2492)

Also paging @james-d-mitchell as he wrote the code in question, and may have some thoughts or knows restrictions I am not aware of.

@stevelinton
Copy link
Contributor

Perhaps the most "correct" option would be to use T_DATOBJ and set the type to TYPE_KERNEL_OBJECT

@james-d-mitchell
Copy link
Contributor

The most straightforward way to me would seem to be option 1. I guess that it was not done this way originally because the layout of transformations and partial perms was different at some point.

fingolfin added a commit to fingolfin/gap that referenced this issue May 28, 2018
fingolfin added a commit that referenced this issue May 28, 2018
ChrisJefferson pushed a commit to ChrisJefferson/gap that referenced this issue Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants