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: add 'void * ref' to all GC mark functions #5662

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

fingolfin
Copy link
Member

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.

@fingolfin
Copy link
Member Author

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.

@fingolfin
Copy link
Member Author

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

@james-d-mitchell
Copy link
Contributor

Just to say I'm happy to accept a PR and to make a release of Semigroups to facilitate this change!

@ChrisJefferson
Copy link
Contributor

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.

@fingolfin
Copy link
Member Author

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.

@fingolfin
Copy link
Member Author

Semigroups PR is at semigroups/Semigroups#1003

@fingolfin fingolfin marked this pull request as ready for review March 6, 2024 15:32
@fingolfin fingolfin closed this Mar 11, 2024
@fingolfin fingolfin reopened this Mar 11, 2024
@dimpase
Copy link
Member

dimpase commented Mar 11, 2024

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.

@fingolfin
Copy link
Member Author

This should be ready now.

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.
@fingolfin fingolfin force-pushed the mh/MarkBag-with-ref branch 2 times, most recently from b43c15e to 258ec08 Compare March 12, 2024 09:21
@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Mar 13, 2024
@fingolfin
Copy link
Member Author

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.

@fingolfin fingolfin merged commit 2b0e78d into gap-system:master Mar 13, 2024
25 checks passed
@fingolfin fingolfin deleted the mh/MarkBag-with-ref branch March 13, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants