julia_gc: include julia.h globally for better CHANGED_BAG - #6490
Open
fingolfin wants to merge 1 commit into
Open
julia_gc: include julia.h globally for better CHANGED_BAG#6490fingolfin wants to merge 1 commit into
julia.h globally for better CHANGED_BAG#6490fingolfin wants to merge 1 commit into
Conversation
fingolfin
added
kind: enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6490 +/- ##
==========================================
- Coverage 79.01% 78.98% -0.03%
==========================================
Files 683 683
Lines 294105 294106 +1
Branches 8656 8655 -1
==========================================
- Hits 232379 232306 -73
- Misses 59919 59992 +73
- Partials 1807 1808 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fingolfin
marked this pull request as draft
August 5, 2026 21:23
Member
Author
|
This may need some more refining... |
lgoettgens
pushed a commit
to oscar-system/GAP.jl
that referenced
this pull request
Aug 10, 2026
`THREADSAFE_GAP_JL` is never defined, so `BEGIN_GAP_SYNC` and `END_GAP_SYNC` always expanded to no-ops, and `BeginGapSync` and `EndGapSync` were compiled into the kernel extension without ever being declared or called. The one function that did run, `InitGapSync`, merely initialized a mutex nobody locks and set a flag nobody reads. It also declared `extern int jl_n_threads` itself, which since Julia 1.9 conflicts with the `_Atomic(int)` declaration in julia.h. That went unnoticed as long as julia.h was not in scope in this file, but it breaks the build once GAP's src/gasman.h includes julia.h, see gap-system/gap#6490. AI disclosure: Claude Code (Opus 5) diagnosed the issue, drafted the change, and verified it locally. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Member
|
oscar-system/GAP.jl#1412 is now available in a GAP.jl release |
fingolfin
force-pushed
the
mh/julia-gc-global-header
branch
from
August 15, 2026 19:36
674a54f to
7ff0308
Compare
fingolfin
force-pushed
the
mh/julia-gc-global-header
branch
2 times, most recently
from
August 28, 2026 07:24
5629d9c to
d24abca
Compare
fingolfin
marked this pull request as ready for review
August 28, 2026 11:12
fingolfin
force-pushed
the
mh/julia-gc-global-header
branch
from
August 28, 2026 11:16
d24abca to
407641f
Compare
Member
Author
|
This should be ready now |
lgoettgens
reviewed
Aug 31, 2026
Comment on lines
+188
to
+189
| // Julia's headers also define FORCE_INLINE, so get rid of that first | ||
| #undef FORCE_INLINE |
Member
There was a problem hiding this comment.
This seems quite a random place to do this. Wouldn't it be more sensible to put it right after including a julia header?
Member
Author
There was a problem hiding this comment.
There is a single file in GAP which uses FORCE_INLINE, namely this one so it's not that random... That said, we then should just get rid for FORCE_INLINE here. Which I just did
Comment on lines
180
to
182
Member
There was a problem hiding this comment.
unrelated observation: these three lines already appear in lines 171-173 right above
fingolfin
force-pushed
the
mh/julia-gc-global-header
branch
from
August 31, 2026 21:45
407641f to
cf51c97
Compare
CHANGED_BAG is supposed to have minimal overhead, which normally is achieved by inlining it. But we never had this for the Julia GC, mainly because it was painful to do so in our C++ source files. This PR addresses this. Unfortunately, simply inlining CHANGED_BAG by calling jl_gc_wb_back does not work: Julia declares that function static inline, and C forbids referencing an identifier with internal linkage from an inline function with external linkage. Making CHANGED_BAG itself static inline is not an option either, as it is used by other EXPORT_INLINE functions, which then run into the very same problem; and turning all of those into static inline would drop symbols from libgap which GAP.jl relies on. So instead inline a copy of Julia's write barrier, which only refers to jl_gc_queue_root. Since src/gasman.h now includes julia.h, packages need the Julia headers as well, so pass them on via sysinfo.gap. The flags reported by julia-config.jl are quoted, which does not survive the way sysinfo.gap is consumed, hence strip the quotes; and resolve the clash between Julia's FORCE_INLINE and the one used by our copy of MurmurHash3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fingolfin
force-pushed
the
mh/julia-gc-global-header
branch
from
August 31, 2026 21:46
cf51c97 to
ed1db4a
Compare
fingolfin
enabled auto-merge (squash)
August 31, 2026 22:00
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CHANGED_BAGis supposed to have minimal overhead, which normally is achieved by inlining it. But we never had this for the Julia GC, mainly because it was painful to do so in our C++ source files. This PR addresses this.As a side effect, this enables future work I have planned that will need the Julia headers globally.
Note that
CHANGED_BAGcannot simply calljl_gc_wb_back: Julia declares that asstatic inline, and C forbids referencing an identifier with internal linkage from an inline function with external linkage. MarkingCHANGED_BAGasstatic inlineinstead is not an option either, as it is used by otherEXPORT_INLINEfunctions (such asPushPlist), which then run into the very same problem. So we inline a copy of Julia's write barrier, which only refers tojl_gc_queue_root.Since
src/gasman.hnow includesjulia.h, all GAP kernel extensions need the Julia headers, too, so we pass those on to packages viasysinfo.gap. TheJULIA=yesCI job now also runstestmockpkgto catch regressions here.This also means that macros defined by
julia.hcan now clash with code in kernel extensions. One such clash affects GAP.jl'sJuliaInterface, which declaresjl_n_threadsitself; theCI with GAP.jljobs will therefore keep failing until oscar-system/GAP.jl#1412 has been merged.