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 =============================================================
status-firefox59: --- → affected
Priority: -- → P1
This is OOM, but there may be some things we can do to mitigate this.
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
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.
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.
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+
Pushed by email@example.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
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
Backed out for perf and memory regressions: https://hg.mozilla.org/integration/mozilla-inbound/rev/12a50b0af80f3c82a63dc408736ea17192d63f3d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/12a50b0af80f
Memory use seems to have returned to previous levels.
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago → a month ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
You need to log in before you can comment on or make changes to this bug.