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

REOPENED
Assigned to

Status

()

Core
JavaScript: GC
P1
critical
REOPENED
2 months ago
a month ago

People

(Reporter: jseward, Assigned: jonco)

Tracking

({crash})

unspecified
mozilla59
Unspecified
Windows 10
crash
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 months ago
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&lt;js::gc::StoreBuffer::MonoTypeBuffer&lt;js::gc::StoreBuffer::CellPtrEdge&gt;, js::gc::StoreBuffer::CellPtrEdge&gt; 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&lt;js::jit::ICCall_Scripted, js::jit::ICStubSpace*&amp;, js::jit::JitCode*, js::jit::ICStub*&amp;, JS::Rooted&lt;JSFunction*&gt;&amp;, JS::Rooted&lt;JSObject*&gt;&amp;, unsigned int&amp;&gt; 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 

=============================================================
(Reporter)

Updated

2 months ago
Flags: needinfo?(jcoppeard)
status-firefox59: --- → affected
Priority: -- → P1
(Assignee)

Comment 1

2 months ago
This is OOM, but there may be some things we can do to mitigate this.
(Assignee)

Comment 2

2 months ago
Created attachment 8935004 [details] [diff] [review]
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.)
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #8935004 - Flags: review?(sphink)
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.
Attachment #8935004 - Flags: review?(sphink)
(Assignee)

Comment 4

2 months ago
Created attachment 8935410 [details] [diff] [review]
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.
Attachment #8935004 - Attachment is obsolete: true
Attachment #8935410 - Flags: review?(sphink)
(Assignee)

Comment 5

2 months ago
Created attachment 8935412 [details] [diff] [review]
bug1422575-storebuffer-oom

As before, pre-allocate the store buffer hash sets.
Attachment #8935412 - Flags: review?(sphink)
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?
Attachment #8935410 - Flags: review?(sphink) → review+
Attachment #8935412 - Flags: review?(sphink) → review+

Comment 7

2 months ago
Pushed by jcoppeard@mozilla.com:
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/115d70e6c818
https://hg.mozilla.org/mozilla-central/rev/1938afc34193
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1424324
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.
status-firefox57: --- → wontfix
status-firefox58: --- → wontfix
status-firefox-esr52: --- → wontfix
(Assignee)

Comment 10

a month ago
Backed out for perf and memory regressions:

https://hg.mozilla.org/integration/mozilla-inbound/rev/12a50b0af80f3c82a63dc408736ea17192d63f3d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

a month ago
Memory use seems to have returned to previous levels.
Status: REOPENED → RESOLVED
Last Resolved: 2 months agoa month ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 13

a month ago
Wrong bug.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
You need to log in before you can comment on or make changes to this bug.