Closed Bug 1380030 Opened 3 years ago Closed 3 years ago

Mark color constants refer to previous mark bit encoding


(Core :: JavaScript: GC, enhancement)

55 Branch
Not set



Tracking Status
firefox56 --- fixed


(Reporter: jonco, Assigned: jonco)



(3 files)

We still make use of BLACK and GRAY mark colors when getting the mark state of a cell (the isMarked methods).  These refer to the previous mark bit encoding and have the following meaning:

  BLACK: black or gray
  GRAY:  gray

This is confusing and should be cleaned up.
The isMarked() methods on Cell/TenuredCell takes a color, but outside of assertions this is always known at compile time.  This patch replaces these methods with isMarkedBlack(), isMarkedGray() and isMarkedAny().

Note that calls to isMarked(BLACK) (or just isMarked() since that's the default) are equivalent to isMarkedAny().
Attachment #8885636 - Flags: review?(sphink)
Simplify and refactor calls to the isMarked() methods.  isMarkedAny() is replaced with isMarkedBlack() where that's what was intended and |isMarkedAny() && !isMarkedGray()| is replaced with isMarkedBlack().  The latter check is more efficient now since it doesn't need to check the gray bit at all.  Also use of asTenured() can be removed since these methods are present on Cell now.
Attachment #8885637 - Flags: review?(sphink)
Finally, move the mark color constants out of the public API and replace them with an enum.  Also I made a bunch of methods take TenuredCell where appropriate.
Attachment #8885639 - Flags: review?(sphink)
Comment on attachment 8885636 [details] [diff] [review]

Review of attachment 8885636 [details] [diff] [review]:

::: js/public/HeapAPI.h
@@ +62,5 @@
>   * depends on the size of the GCThing. Objects marked gray are eligible for
>   * cycle collection.
>   */
>  static const uint32_t BLACK = 0;
>  static const uint32_t GRAY = 1;

Do these get used elsewhere in ways that are now incorrect?

@@ +66,5 @@
>  static const uint32_t GRAY = 1;
>  /*
>   * Two bits determine the mark color as follows:
> + *    BlackBit      GrayOrBlackBit   color

Naming is a little weird. When GrayOrBlack is zero, it might still be gray or black (specifically, black). It's really GrayIfNotBlackBit or AtLeastGrayBit. But I think it's ok.

::: js/src/gc/Marking.cpp
@@ +310,5 @@
>           * Having black->gray edges violates our promise to the cycle
>           * collector. This can happen if we're collecting a compartment and it
>           * has an edge to an uncollected compartment: it's possible that the
>           * source and destination of the cross-compartment edge should be gray,
>           * but the source was marked black by the conservative scanner.

We really ought to eliminate this "conservative scanner" thing.

@@ +3461,2 @@
>          return MarkInfo::GRAY;
> +    if (cell->isMarkedAny())

I was just going to comment that I hoped this would optimize away the redundant bit check, when I realized this is my debug thing.

::: js/src/shell/js.cpp
@@ +5826,2 @@
>                  color = "gray";
> +            else if (cell->isMarkedAny())

wouldn't isMarkedBlack() read better here?
Attachment #8885636 - Flags: review?(sphink) → review+
Comment on attachment 8885637 [details] [diff] [review]

Review of attachment 8885637 [details] [diff] [review]:

::: js/src/gc/Marking.cpp
@@ +3462,2 @@
>          return MarkInfo::BLACK;
> +    MOZ_ASSERT(!cell->isMarkedAny());

I'd rather not assert here. If you want to check this, then I'd add MarkInfo::BOGUS or something. This is intended only for use via manual calls within gdb, and I'd rather that not crash.

::: js/src/jsfriendapi.cpp
@@ +1107,5 @@
> +    if (cell->isMarkedBlack())
> +        return 'B';
> +    if (cell->isMarkedGray())
> +        return 'G';
> +    MOZ_ASSERT(!cell->isMarkedAny());

Hm... I'm not sure which is better. 'X' is intended as a sign of a bogus heap state. If you're dumping the heap, would you want that or an assert? I'm not sure.
Attachment #8885637 - Flags: review?(sphink) → review+
Comment on attachment 8885639 [details] [diff] [review]

Review of attachment 8885639 [details] [diff] [review]:

I kind of skimmed this, but it looks good.

::: js/public/HeapAPI.h
@@ -62,5 @@
> - * depends on the size of the GCThing. Objects marked gray are eligible for
> - * cycle collection.
> - */
> -static const uint32_t BLACK = 0;
> -static const uint32_t GRAY = 1;

Ah! Ignore my earlier comment, then.
Attachment #8885639 - Flags: review?(sphink) → review+
Pushed by
Refactor isMarked() methods into separate methods for each color and any r=sfink
Simplify and refactor use of isMarked*() methods r=sfink
Remove color constants from public API and replace with an internal MarkColor enum r=sfink
You need to log in before you can comment on or make changes to this bug.