Closed Bug 875304 Opened 9 years ago Closed 9 years ago

Non-fatal assertions and telemetry for cycle collector OOMs

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(4 files, 1 obsolete file)

Rather than warning many times.  This will make it easier to track when this happens on TBPL.
We could also just MOZ_CRASH when the CC OOMs.  Though maybe it would be a good idea to collect telemetry about how much this happens (using a flag, so if somebody repeatedly OOMs it will only show up once) before doing that.
Blocks: 867974
Depends on: 875557
Whiteboard: [MemShrink]
Summary: Non-fatally assert once when the CC hits an OOM state → Non-fatal assertions and telemetry for cycle collector OOMs
Update for infallible-by-default nsDeque.
Attachment #753985 - Attachment is obsolete: true
I should change part 1 to take advantage of the fact that nsTArray::AppendElement is infallible:
        if (pinfo->mColor == white && mWhiteNodes->AppendElement(pinfo)) {
            rv = pinfo->mParticipant->Root(pinfo->mPointer);
(In reply to Andrew McCreight [:mccr8] from comment #7)
> I should change part 1 to take advantage of the fact that
> nsTArray::AppendElement is infallible:
>         if (pinfo->mColor == white && mWhiteNodes->AppendElement(pinfo)) {
>             rv = pinfo->mParticipant->Root(pinfo->mPointer);

We should really make the infallible AppendElement return void...
I'm not sure how that would work, given that the fallibility is based on a parameter to the class, but I'm not a C++ guru.  It does sound like a good idea.
(In reply to comment #9)
> I'm not sure how that would work, given that the fallibility is based on a
> parameter to the class, but I'm not a C++ guru.

Using black magic :D  See bug 853548 where I did this for SetCapacity.

> It does sound like a good idea.

Hmm, actually tbsaunde correctly mentioned on IRC that the APIs returning pointers might be a good idea too, at least, if the pointer is used to do something useful (not check for fallibility...)  So, I'm sort of torn on what is the best idea to be honest.
Attachment #757486 - Flags: review?(bugs)
Attachment #753982 - Flags: review?(bugs)
I need to test parts 3 and 4 before putting them up for review.  For instance, I haven't checked that they do anything when you OOM.
Attachment #753982 - Flags: review?(bugs) → review+
Attachment #757486 - Flags: review?(bugs) → review+
Whiteboard: [MemShrink] → [MemShrink:P2]
Attachment #753983 - Flags: review?(bugs)
For part 3, we're actually running on Nightly with fatal OOMs in nsDeque, and nobody has complained, and they don't show up on crash stats as far as I can tell, so I'm kind of tempted to just leave it alone as is... I'll think about it some more.
Attachment #753983 - Flags: review?(bugs) → review+
I'll spin off part 3 into a new bug.  That's probably worth fixing, as I've seen 1 TBPL orange from hitting nsDeque OOM during shutdown.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][leave open] → [MemShrink:P2]
Target Milestone: --- → mozilla24
Comment on attachment 755113 [details] [diff] [review]
part 3: add non-fatal assertions and telemetry when nsDeque fails to push during graph scanning

I filed bug 887903 for making nsDeque::Push fallible in the CC.
Attachment #755113 - Flags: checkin-
You need to log in before you can comment on or make changes to this bug.