Bug 138369 - Rename checkMarkByte() to checkIfEitherInEdenGenOrAlreadyRemembered()
Summary: Rename checkMarkByte() to checkIfEitherInEdenGenOrAlreadyRemembered()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-04 13:36 PST by Mark Lam
Modified: 2014-11-04 17:20 PST (History)
0 users

See Also:


Attachments
the patch. (13.46 KB, patch)
2014-11-04 13:43 PST, Mark Lam
ggaren: review-
Details | Formatted Diff | Diff
take 2 (14.05 KB, patch)
2014-11-04 15:45 PST, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2014-11-04 13:36:47 PST
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.
Comment 1 Mark Lam 2014-11-04 13:43:44 PST
Created attachment 240945 [details]
the patch.
Comment 2 Geoffrey Garen 2014-11-04 14:59:56 PST
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.
Comment 3 Mark Lam 2014-11-04 15:45:40 PST
Created attachment 240969 [details]
take 2
Comment 4 Mark Lam 2014-11-04 15:47:02 PST
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.
Comment 5 Geoffrey Garen 2014-11-04 17:03:08 PST
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
Comment 6 Mark Lam 2014-11-04 17:20:22 PST
(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>.