817343 - GC validation isn't happening

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

We're not actually calling ValidateIncrementalMarking right now. It only runs if gcCompartmentGroupIndex == 0. However, the first group to be swept has that set to 1, since we call GetNextCompartmentGroup in BeginSweepPhase.

When I fixed this, the checks fail. I noticed that we're no longer in a good position to check gray marking anymore, since at best we'd only be checking the first compartment group anyway. So I took these checks out.

It's also a bit troublesome to check weak marking, since that's now done on a per-compartment group basis. This has never found any bugs, and it requires some annoying code to deal with saving and restoring the weakmap list. So I took that out too.

With these changes, the checks seem to pass.

Comment 1

[Jon Coppeard (:jonco)]

Comment on attachment 687458 [details] [diff] [review]
patch

Review of attachment 687458 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I realise I broke this.  Fix looks good, it's a shame we can't assert the gray marking worked though.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1ee5f59a6e

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Had to back this out because it was occasionally turning browser-chrome tests orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/46ac86b232eb

Some of the logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17566086&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=17566697&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=17566358&tree=Mozilla-Inbound

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Jon, if you have some extra time, these assertions are interesting. They also might be related to bug 817342.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Alternative patch to validate every compartment group

This is really inefficient because it does a full non-incremental mark for every compartment group, although this could be improved.  

Also not heavily tested.

Try push is here: https://tbpl.mozilla.org/?tree=Try&rev=a5dd2304aafb

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Alternative patch to validate every compartment group

Here's an updated version of this patch.  Still expecting some failures but should not be so inefficient as the marking's only done once.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Validate every compartment group

I've removed all gray marking validation from this one, and I'm going to try and address that in a separate patch.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 690444 [details] [diff] [review]
Validate every compartment group

Review of attachment 690444 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, but I'd like to see how you handle the MarkWeakReferences issue.

::: js/src/jsgc.cpp
@@ +2754,5 @@
> +
> +namespace js {
> +namespace gc {
> +
> +class MarkingValidator

As long as this has been forward-declared, I think you can do |class js::gc::MarkingValidator| and remove the namespace scopes.

@@ +2780,5 @@
> +{}
> +
> +js::gc::MarkingValidator::~MarkingValidator()
> +{
> +    if (map.initialized()) {

Could you do this as:
  if (!map.initialized())
      return;

@@ +2781,5 @@
> +
> +js::gc::MarkingValidator::~MarkingValidator()
> +{
> +    if (map.initialized()) {
> +        for (BitmapMap::Range r(map.all()); !r.empty(); r.popFront()) {

No braces needed.

@@ +2808,2 @@
>          ChunkBitmap *bitmap = &r.front()->bitmap;
> +	ChunkBitmap *entry = new ChunkBitmap();

Please use |js_new<ChunkBitmap>()| here.

@@ +2822,5 @@
>      WeakMapVector weakmaps;
> +    ArrayBufferVector arrayBuffers;
> +    for (GCCompartmentsIter c(runtime); !c.done(); c.next()) {
> +        if (!WeakMapBase::saveCompartmentWeakMapList(c, weakmaps) ||
> +            !ArrayBufferObject::saveArrayBufferList(c, arrayBuffers)) {

The brace should go on its own line since the condition takes up two lines.

@@ +2864,5 @@
> +    runtime->gcMarker.drainMarkStack(budget);
> +
> +    {
> +        gcstats::AutoPhase ap(runtime->gcStats, gcstats::PHASE_SWEEP);
> +        MarkWeakReferences(runtime, gcstats::PHASE_SWEEP_MARK_WEAK);

This function uses CompartmentGroupIter, so I don't think it's doing what we need here. You need a special version that iterates over all compartments.

One terrifying suggestion would be to templatize MarkWeakReferences on the iterator class :-).

@@ +2875,5 @@
> +        ChunkBitmap *entry = map.lookup(chunk)->value;
> +
> +	ChunkBitmap tmp = *entry;
> +        *entry = *bitmap;
> +        *bitmap = tmp;

I think you can use js::Swap (from jsutil.h) to make this a little clearer.

@@ +2900,5 @@
> +
> +    if (!initialized)
> +        return;
> +
> +    unsigned count = 0;

This looks dead.

@@ +2944,4 @@
>  #endif
> +
> +static void
> +ComputeNonIncrementalMarking(JSRuntime *rt)

Maybe call this ComputeNonIncrementalMarkingForValidation? It's long, but we only call it once.

@@ +2948,5 @@
> +{
> +#ifdef DEBUG
> +    JS_ASSERT(!rt->gcMarkingValidator);
> +    if (rt->gcIsIncremental && rt->gcValidate)
> +        rt->gcMarkingValidator = new MarkingValidator(rt);

We're not allowed to use |new| directly because of exception safety or something. Please replace with |js_new<MarkingValidator>(rt)|.

@@ +3516,5 @@
>  static bool
>  SweepPhase(JSRuntime *rt, SliceBudget &sliceBudget)
>  {
> +#ifdef DEBUG
> +    MarkingValidator validator(rt);

What's this for?

::: js/src/jstypedarray.h
@@ +135,5 @@
>      static void sweep(JSCompartment *rt);
>  
>      static void resetArrayBufferList(JSCompartment *rt);
>  
> +    static bool saveArrayBufferList(JSCompartment *c, ArrayBufferVector &vector);

Could you remove the blank lines between reset, save, and restore?

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Validate every compartment group

New patch with review comments addressed.

Thanks for the idea about templating on the iterator class, I would not have thought of that ;)  But actually it expresses the intent, and I can't think of a cleaner alternative.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 690851 [details] [diff] [review]
Validate every compartment group

Review of attachment 690851 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thanks!

::: js/src/jsgc.cpp
@@ +2697,5 @@
>  
>      return true;
>  }
>  
> +template <class CompartmentIter>

This name is a little confusing given the existing CompartmentsIter. How about calling it CompartmentIterT? That will make people notice it more.

Comment 11

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/421ea4f8b050

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/421ea4f8b050