875304 - Non-fatal assertions and telemetry for cycle collector OOMs

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Rather than warning many times.  This will make it easier to track when this happens on TBPL.

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 1: remove pointless checks for |new| failures

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 2: add non fatal assertions and telemetry for OOM during CC graph building

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 3: add non-fatal assertions and telemetry when nsDeque fails to push during graph scanning

Comment 5

[Andrew McCreight [:mccr8]]

try run was okay https://tbpl.mozilla.org/?tree=Try&rev=b059c667e817

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 3: add non-fatal assertions and telemetry when nsDeque fails to push during graph scanning

Update for infallible-by-default nsDeque.

Comment 7

[Andrew McCreight [:mccr8]]

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);

Comment 8

[(no longer active)]

(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...

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[(no longer active)]

(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.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 1b: remove pointless checks for nsTArray::AppendElement failure

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[Andrew McCreight [:mccr8]]

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.

Comment 14

[Andrew McCreight [:mccr8]]

This was the try run: https://tbpl.mozilla.org/?tree=Try&rev=a6440054ae23

Comment 15

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f629bdc6b38
https://hg.mozilla.org/integration/mozilla-inbound/rev/92c6423211e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/519f9fdd9544

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6f629bdc6b38
https://hg.mozilla.org/mozilla-central/rev/92c6423211e7
https://hg.mozilla.org/mozilla-central/rev/519f9fdd9544

Comment 17

[Andrew McCreight [:mccr8]]

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.

Comment 18

[Andrew McCreight [:mccr8]]

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.