Mark color constants refer to previous mark bit encoding

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript: GC
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

55 Branch
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

8 months ago
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.
(Assignee)

Comment 1

8 months ago
Created attachment 8885636 [details] [diff] [review]
refactor-is-marked

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)
(Assignee)

Comment 2

8 months ago
Created attachment 8885637 [details] [diff] [review]
refactor-is-marked-2

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)
(Assignee)

Comment 3

8 months ago
Created attachment 8885639 [details] [diff] [review]
refactor-is-marked-3

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]
refactor-is-marked

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]
refactor-is-marked-2

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]
refactor-is-marked-3

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+

Comment 7

8 months ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2339ab06af5
Refactor isMarked() methods into separate methods for each color and any r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/04364ec85017
Simplify and refactor use of isMarked*() methods r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/933aa2989b9a
Remove color constants from public API and replace with an internal MarkColor enum r=sfink

Comment 8

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e2339ab06af5
https://hg.mozilla.org/mozilla-central/rev/04364ec85017
https://hg.mozilla.org/mozilla-central/rev/933aa2989b9a
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.