1422575 - Crash in OOM | unknown | js::AutoEnterOOMUnsafeRegion::crash | js::gc::StoreBuffer::put<T>

Status: RESOLVED WORKSFORME

(Core :: JavaScript: GC, defect, P1)

Description

[Julian Seward [:jseward]]

This bug was filed from the Socorro interface and is
report bp-ac1b9ecf-861a-4edb-9abf-487340171201.
=============================================================

This is topcrash #8 in the windows nightly 20171130220131.

Top 8 frames of crashing thread:

0 xul.dll js::AutoEnterOOMUnsafeRegion::crash js/src/jscntxt.cpp:1651
1 xul.dll js::gc::StoreBuffer::put<js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::CellPtrEdge>, js::gc::StoreBuffer::CellPtrEdge> js/src/gc/StoreBuffer.h:372
2 xul.dll js::jit::ICCall_Scripted::ICCall_Scripted js/src/jit/BaselineIC.cpp:4672
3 xul.dll js::jit::ICStubCompiler::newStub<js::jit::ICCall_Scripted, js::jit::ICStubSpace*&, js::jit::JitCode*, js::jit::ICStub*&, JS::Rooted<JSFunction*>&, JS::Rooted<JSObject*>&, unsigned int&> js/src/jit/SharedIC.h:1131
4 xul.dll js::jit::TryAttachCallStub js/src/jit/BaselineIC.cpp:2306
5 xul.dll js::jit::DoCallFallback js/src/jit/BaselineIC.cpp:2529
6  @0x144f5f77621 
7 xul.dll xul.dll@0xb0112b 

=============================================================

Comment 1

[Jon Coppeard (:jonco)]

This is OOM, but there may be some things we can do to mitigate this.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1422575-storebuffer-oom

Here's a patch to pre-allocate the hash sets used by the store buffer.  We  request a minor GC before we reach the actual capacity is reached since the size of the hash table is actually a multiple of the initial size requested.

However it's kind of nasty adding a special removeWithoutResize() API just for this purpose.  I considered adding another template parameter for minimum hash table size, but that's not great either.  What do you think?

(Also this patch makes the size of the hash set the same on 32 and 64 bit platforms which it wasn't before.  The value I picked was about half way between the original sizes for the different platforms.)

Comment 3

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

Comment on attachment 8935004 [details] [diff] [review]
bug1422575-storebuffer-oom

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

How is this showing up as the #8 crasher?!

The API addition is unfortunate. Especially since if you need a barriered table, there's yet another entry point to consider.

Also, it seems a little weird to have a storebuffer use a hashtable in a different way in order to get the nonshrinking behavior. I guess it seems better to me to have a nonshrinking hashtable. This seems like it wants to add a new invariant, and that's never going to work (though it'll probably work well enough) without the hashtable implementation maintaining it. For example, if someone calls clearAndShrink(), it'll happily shrink (which is probably fine, since you explicitly told it to.) But more problematic, if you ever remove something using an Enum, currently it could also shrink. You'd need a NonShrinkingEnum or something to plug that hole. Which isn't a fatal flaw, you're just saying that it's up to the user to indirectly manage the table's minimum capacity by not calling things that would shrink it, but it's nonobvious what that means.

What do you think of adding a minCapacity field, initialized to sMinCapacity but controllable with a setMinCapacity() or something? Hopefully you could mostly then just s/sMinCapacity/minCapacity/, though it does look like there's some funky logic around picking the actual capacity in init(). But maybe using it in wouldBeUnderloaded() is enough.

We really need to switch hashtable implementations, I think. froydnj is doing this for the Gecko one, switching to a Robin Hood hashing variant. Oh well; different bug.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1422575-hashtable-min-capacity

Patch to give hash tables a minimum capacity based on the value passed to init().

This makes the generation field debug-only; it was never used outside debug builds anyway.  The space is repurposed for the minCapacity field.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1422575-storebuffer-oom

As before, pre-allocate the store buffer hash sets.

Comment 6

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

Comment on attachment 8935410 [details] [diff] [review]
bug1422575-hashtable-min-capacity

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

lgtm, thanks!

::: js/public/HashTable.h
@@ +1366,5 @@
>                 capacity() * sMaxAlphaNumerator / sAlphaDenominator;
>      }
>  
>      // Would the table be underloaded if it had the given capacity and entryCount?
> +    bool wouldBeUnderloaded(uint32_t capacity)

The comment probably ought to be updated. Maybe

// Considering its current entryCount, would the table be underloaded if it had the given capacity?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/115d70e6c818
Give hash tables a minimum capacity based on the parameter passed to init() r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/1938afc34193
Give store buffer hash sets a minimum capacity to reduce the chance of OOM while adding entries r=sfink

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/115d70e6c818
https://hg.mozilla.org/mozilla-central/rev/1938afc34193

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Crash volume doesn't look worth backport consideration, but feel free to set the status for 58 back to affected and request approval if you feel strongly otherwise.

Comment 10

[Jon Coppeard (:jonco)]

Backed out for perf and memory regressions:

https://hg.mozilla.org/integration/mozilla-inbound/rev/12a50b0af80f3c82a63dc408736ea17192d63f3d

Comment 11

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

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/12a50b0af80f

Comment 12

[Jon Coppeard (:jonco)]

Memory use seems to have returned to previous levels.

Comment 13

[Jon Coppeard (:jonco)]

Wrong bug.

Comment 14

[Jason Orendorff [:jorendorff]]

This crash signature seems to be gone in 63.