Closed Bug 1447074 Opened 3 years ago Closed 3 years ago

Improve assertions around the whole cell store buffer


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




Tracking Status
firefox61 --- fixed


(Reporter: jonco, Assigned: jonco)



(1 file)

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.
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.
Attachment #8960510 - Flags: review?(sphink)
Comment on attachment 8960510 [details] [diff] [review]

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?
Attachment #8960510 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #2)
Good ideas.  I'll factor out checkEmpty() and rename to minorGCNumberAtCreation.
Pushed by
Improve assertions for the whole cell store buffer r=sfink
Priority: -- → P2
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.