gap
gap copied to clipboard
gap-system
kernel: add 'void * ref' to all GC mark functions
This helps the Julia GC to pass along certain global state through marking functions to MarkBag, instead of needing to use global variables for that (which is not thread safe).
For GASMAN and Boehm, this does nothing.
The main downside is that it breaks kernel extensions defining custom marking functions. But as far as I can tell, that only affects Semigroups, for which I am confident we can get a patch in that will make it work with this while staying compatible with older kernel versions. Enabling this is why I've added
#define GAP_MARK_FUNC_WITH_REF 1
to this PR: then one can add something like
#ifdef GAP_MARK_FUNC_WITH_REF
#define MarkBag(obj,ref) MarkBag(obj)
#endif
to code that needs to stay compatible with GAP versions before and after this patch.
This is still a draft. In particular, the Julia versions crashes right now 😂 but I created this some time ago late at night, and didn't have time to debug this so far. But I don't want to forget it as it will help me resolve threading issues with the Julia GC. As this is breaking the kernel ABI, it would be kinda cool if this could still go into GAP 4.13.0 (otherwise it would have to wait for 4.14.x...). But of course that depends on how quick it can be made to work, and how quick I can then submit a patch to @james-d-mitchell for updating semigroups, and whether we can update that in time, and ...
Anyway, I now plan to have GAP 4.13.0 released at the latest during GAP Days, so if this is not ready then, it's out.
Besides fixing the Julia GC integration, also many comments for functions need to be adjusted to mention the new argument. But that's for last, when it works, and when someone else (perhaps @ChrisJefferson) has vetted the general idea.
Ah I think I found the "bug" causing Julia to crash: I had commented out a jl_init call (which needs to be commented out when running GAP from Julia (as opposed to running Julia from GAP). I should find a better solution for that...
Just to say I'm happy to accept a PR and to make a release of Semigroups to facilitate this change!
This looks very reasonable to me, and I agree the only package which looks like it needs a fix is semigroups -- who thanks to @james-d-mitchell and others are very quick at responding and getting new versions out, so I think this should be fine to be in 4.13.
I think the PR is complete, but if it turns out there is some place we aren't forwarding the extra 'ref' properly, I'm sure this includes all API changes correctly.
One thing that is missing (besides adjusting comments): SetExtraMarkFuncBags and related code must be adjusted, so that a suitable ref value is passed to the callback function installed this way. Otherwise it is impossible to call MarkBag correctly from in there.
Alternatively, we could leave it as is and instead drop the ref argument from the libgap API GAP_MarkBag with the following argument: right now the only code using this is SageMath (and possibly that other unfinished alternate GAP-python bridge), and the all only work with GASMAN anyway (definitely not with the Julia GC), so there is no point in burdening that API with this right now.
(Of course perhaps in the future this all changes -- but then that change will be API/ABI breaking anyway, and we can still add the ref argument to GAP_MarkBag.
Perhaps @dimpase or @embray have any thoughts on that.
Unfortunately @embray 's gappy is bitrotting and unfinished.
I'll be looking at fixing SageMath's "old" libgap interface, as soon as there is GAP close enough to a release.
This is a fairly serious change and yet I labeled this as "release notes: not needed". My rationale is that for regular users, and even 99% of package authors, this is relevant. Really only James needed to know, and he already dealt with it. The way I now made it, libgap users also don't need a change. And finally, while it is ABI breaking, well 4.13.0 already is ABI breaking, so there is nothing new, really.