Closed
Bug 1447074
Opened 7 years ago
Closed 7 years ago
Improve assertions around the whole cell store buffer
Categories
(Core :: JavaScript: GC, enhancement, P2)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
Details
Attachments
(1 file)
9.59 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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 2•7 years ago
|
||
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?
Attachment #8960510 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #2)
Good ideas. I'll factor out checkEmpty() and rename to minorGCNumberAtCreation.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9781cbcd9f8
Improve assertions for the whole cell store buffer r=sfink
Updated•7 years ago
|
Priority: -- → P2
Comment 5•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•