kernel: use more accurate marking #2408
Merged
Conversation
This file contains 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
fingolfin
added
topic: kernel
release notes: not needed
fingolfin
force-pushed
the
mh/gc-mark-bags
branch
from
bcc7380
to
12740ef
Compare
Codecov Report
@@ Coverage Diff @@
## master #2408 +/- ##
==========================================
+ Coverage 73.88% 73.88% +<.01%
==========================================
Files 484 484
Lines 245435 245458 +23
==========================================
+ Hits 181333 181361 +28
+ Misses 64102 64097 -5
|
ChrisJefferson
approved these changes
Apr 26, 2018
|
Looks good to me. Just in case anyone has the same thought as me, I wondered about only checking up to |
fingolfin
force-pushed
the
mh/gc-mark-bags
branch
from
12740ef
to
2f8df9c
Compare
If DEBUG_GASMAN_MARKING is #defined, then we increment a counter whenever `MarkBag` is called on something that isn't either zero, or a valid bag ref, or an immediate integer or FFE. By setting a break point on the line incrementing BadMarksCounter, one can quickly find out which bags are affected.
Not all slots of a T_FUNCTION bag are filled with bag refs, yet when marking the slots of such a bag during garbage collection, we still used MarkAllSubBags. This is usually no problem with GASMAN, as it detects if a pointer isn't a master pointer, and can then simply ignore it. But other garbage collections can't as easily verify master pointers. So let's try to be accurate about what we mark as a bag and what not: skip the first eight slots of every T_FUNCTION bag for marking (they contain pointers to C functions), and also ensure that the rest of any T_FUNCTION bag only contains bag refs. Also fix a bug in saving/restoring operations, where the 'enabled' field was stored as an UInt, even though it now is an Obj (though we currently only store immediate ints in it, hence there was no functional problem).
They now all call MarkArrayOfBags, but thanks to inlining, this produces identical machine code.
... instead of a T_BODY, as we don't really create a T_BODY here. (and this can lead to confusion in GC marking)
fingolfin
force-pushed
the
mh/gc-mark-bags
branch
from
2f8df9c
to
cf2c8c1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR how we mark-for-garbage-collection in various bag types with GASMAN. This is motivated by @rbehrends' work on integrating Julia GC, see PR #2092
Specifically, when marking bags during garbage collection, we were very liberal in what we passed on to
MarkBag(directly or indirectly). This is no big deal with GASMAN, as GASMAN can easily decide whether a given pointer is a master pointer or not (it has to be in a certain range), and ignore all other values passed to it. For other GCs, this is not quite as easy, as master pointers typically are not allocated from a fixed pool (though one could think about doing that... with the provision that one may have to maintain multiple such pools...)This PR resolves most of these issues:
T_BODYbags incode.cto maintain two stacks of statements resp. expressions, which are not bags. But in aT_BODY, the first three slots are expected to be bag refs. This wasn't the case for these stacks. So now we use aT_DATOBJfor them (and make sure to leave its type slot intact)MarkAllSubBags; but the first slot contains the length of the list; fixed via new marking functionMarkAllButFirstSubBagsMarkAllSubBags; but only slot 0 (for component objects), and the odd slots starting with slot 3, contain bag refs. Fixed with a custom marking functionMarkPRecSubBagsT_FUNCTIONobjects viaMarkAllSubBags; but we store pointers to C functions in the first 8 slots, and also stored rawUIntvalues in two other slots. The latter were replaced by immediate integer objects; and a custom marking functionMarkFunctionSubBagswas added, which skips the first 8 slots, and only marks the rest.#define DEBUG_GASMAN_MARKINGwhich enables code that tests ifMarkBagis called on something that is neither a valid bag ref, a valid immediate object (integer or FFE), nor a null pointer. This can be used to track down more instances of the problem type described here, which is much easier than trying to do so while also hooking up a new GC at the same time ;-).Exprin slot 1 which should not marked. I moved it from slot 1 into slot 0, and then made it useMarkAllButFirstSubBags. This also required tweaking the LVars pool code, which I did (it is now also documented and more readable).