Bug 138369 – Rename checkMarkByte() to checkIfEitherInEdenGenOrAlreadyRemembered()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Write barriers are needed for GC Eden collections so that we can scan pointers pointing from old generation objects to eden generation objects.  The barrier currently checks the mark byte in a cell to see if the cell needs to be added to the GC remembered set.  The cell should only be added to the remembered set if:
1. If the cell is in the young generation.  It has no old to eden pointers by definition.
2. If the cell is already in the remembered set.

The barrier current names this check as checkMarkByte().  We should rename it to checkIfEitherInYoungGenOrAlreadyRemembered() to be clearer about its intent.

Similarly, Jump results of this check are currently named ownerNotMarkedOrAlreadyRemembered.  This can be misinterpreted as the owner is not marked or not already remembered.  We should rename it to ownerIsInEdenGenOrAlreadyRemembered which is clearer about what we're really checking for.  The cell being "not marked" implies that it is in the eden gen, which is what we really checking for anyway.

[Mark Lam]

Created attachment 240945 [details]
the patch.

[Geoffrey Garen]

Comment on attachment 240945 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=240945&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        1. If the cell is in the young generation.  It has no old to eden pointers by
> +           definition.

Remove the leading "If", since you prefixed the ":" with "if".

> Source/JavaScriptCore/ChangeLog:15
> +        2. If the cell is already in the remembered set.

Remove the leading "If".

The logic of this paragraph is backwards. You either need to invert the numbered items or invert the statement before the colon.

You need to clarify that #2 is just an optimization strategy, and not a strict requirement. It's totally fine to add the same object to a set twice -- it's just redundant, and we have an efficient way to avoid it.

> Source/JavaScriptCore/ChangeLog:17
> +        The barrier current names this check as checkMarkByte().  We should rename it to

currently

> Source/JavaScriptCore/ChangeLog:25
> +        Similarly, Jump results of this check are currently named
> +        ownerNotMarkedOrAlreadyRemembered.  This can be misinterpreted as the owner is
> +        not marked or not already remembered.  We should rename it to
> +        ownerIsInEdenGenOrAlreadyRemembered which is clearer about the intent of the
> +        check.  What we are really checking for is that the cell is in the eden gen,
> +        which is implied by being "not marked".

It's a good point that the old name has an ambiguous modifier in it. But I think the new name has some ambiguity and verbosity too.

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:251
> +    AssemblyHelpers::Jump ownerIsInEdenGenOrAlreadyRemembered = jit.checkIfEitherInEdenGenOrAlreadyRemembered(owner);

How about

    AssemblyHelpers::Jump ownerIsRememberedOrInEden = jit.jumpIfIsRememberedOrInEden(owner);

> Source/JavaScriptCore/jit/AssemblyHelpers.h:668
> +    // Note: gcData being NotMarked (a value of 1) implies that the cell is in the young GC generation.

I don't understand this comment, and it seems out of place int he assembler. A better way to comment this is probably to list the set of possible bit patterns next to the declaration of GCData in JSCell.h.

[Mark Lam]

Created attachment 240969 [details]
take 2

[Mark Lam]

Thanks for the feedback.  I agree, and the suggested changes have been applied.  I also change the LLINT macro name to skipIfIsRememberedOrInEden, which I think reads better than checkIfIsRememberedOrInEden or the original checkMarkByte.

[Geoffrey Garen]

Comment on attachment 240969 [details]
take 2

View in context: https://bugs.webkit.org/attachment.cgi?id=240969&action=review

> Source/JavaScriptCore/runtime/JSCell.h:152
> +        MarkedAndRemembered = 2, // The object is in the GC's remember set.

remembered set

> Source/JavaScriptCore/runtime/JSCell.h:154
> +        // The object being in the GC's remember set implies that it is also

remembered set

> Source/JavaScriptCore/runtime/JSCell.h:155
> +        // Marked. This is because objects are only added to the remembered sets

remembered set

[Mark Lam]

(In reply to comment #5)
> > Source/JavaScriptCore/runtime/JSCell.h:152
> > +        MarkedAndRemembered = 2, // The object is in the GC's remember set.
> 
> remembered set

Oops.  Thanks for the review.  Fixed and landed in r175593: <http://trac.webkit.org/r175593>.