1447074 - Improve assertions around the whole cell store buffer

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P2)

Description

[Jon Coppeard (:jonco)]

We're seeing a lot of crashes that involve single bits being flipped in an otherwise valid word.  There are a limited number of possible culprits, and one of them is the whole cell store buffer.  We should improve the assertions around this to ensure that it is functioning correctly and investigate other ways in which we can make this more robust.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1447074-whole-cell-assertions

Add some more assertions around the whole cell store buffer, in particular checking that store buffer entries added for a particular minor GC are marked in the next minor GC.  Check invariants whenever we add anything to the buffer.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8960510 [details] [diff] [review]
bug1447074-whole-cell-assertions

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

::: js/src/gc/StoreBuffer.cpp
@@ +41,5 @@
> +    MOZ_ASSERT(bufferVal.isEmpty());
> +    MOZ_ASSERT(bufferCell.isEmpty());
> +    MOZ_ASSERT(bufferSlot.isEmpty());
> +    MOZ_ASSERT(bufferGeneric.isEmpty());
> +    MOZ_ASSERT(!bufferWholeCell);

Can you put this in a StoreBuffer::checkEmpty() or StoreBuffer::assertEmpty()? It seems like an important property to be able to talk about of the overall store buffer. (Really, it makes more sense to have an isEmpty(), but then you'd lose track of which thing was nonempty.)

::: js/src/gc/StoreBuffer.h
@@ +139,5 @@
>          }
>  
> +        bool isEmpty() {
> +            return last_ == T() && (!stores_.initialized() || stores_.empty());
> +        }

I sorta wish last_ were named differently; on first read, I was thinking of this as the last element of a list or something. (Which it sort of is, if you think of the virtual list of added entries, I suppose.) Then again, last_ is a pretty good name for its purpose... maybe lastAdded_ would have been a little better? Oh well, not worth bothering with.

This should really be const. So should GenericBuffer::isEmpty(). StoreSet::initialized() appears to be const already, so I don't see anything blocking it.

@@ +473,5 @@
>  
> +#ifdef DEBUG
> +    // The minor GC number when this was created. This object should not survive
> +    // past the next minor collection.
> +    const uint64_t minorGCNumber;

I wonder if this would be more clear as creationMinorGCNumber? or minorGCNumberAtCreation?

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
Good ideas.  I'll factor out checkEmpty() and rename to minorGCNumberAtCreation.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9781cbcd9f8
Improve assertions for the whole cell store buffer r=sfink

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f9781cbcd9f8