Closed Bug 1254108 Opened 4 years ago Closed 4 years ago

Assertion failure: gcTriggerBytes_ >= amount, at js/src/jsgc.cpp:1924 with shell-only call to gcparam

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: decoder, Assigned: jonco)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(1 file, 1 obsolete file)

The following testcase crashes on mozilla-central revision b6acf4d4fc20 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2):

function testChangeParam(key) {
    gcparam(key, 0x22222222);
}
testChangeParam("lowFrequencyHeapGrowth");



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff5cc3700 (LWP 55739)]
0x00000000008e4cc2 in js::gc::ZoneHeapThreshold::updateForRemovedArena (this=<optimized out>, tunables=...) at js/src/jsgc.cpp:1924
#0  0x00000000008e4cc2 in js::gc::ZoneHeapThreshold::updateForRemovedArena (this=<optimized out>, tunables=...) at js/src/jsgc.cpp:1924
#1  0x00000000008e4d92 in js::gc::GCRuntime::releaseArena (this=0x7ffff695d430, arena=0x7ffff7e66000, lock=...) at js/src/jsgc.cpp:1101
#2  0x00000000008f7b7b in ReleaseArenaList (lock=..., arena=<optimized out>, rt=0x7ffff695d000) at js/src/jsgc.cpp:2867
#3  js::gc::GCRuntime::sweepBackgroundThings (this=this@entry=0x7ffff695d430, zones=..., threadType=threadType@entry=js::BackgroundThread, freeBlocks=...) at js/src/jsgc.cpp:3420
#4  0x00000000008f8075 in sweepBackgroundThings (threadType=js::BackgroundThread, freeBlocks=..., zones=..., this=0x7ffff695d430) at js/src/jsgc.cpp:3661
#5  js::GCHelperState::doSweep (this=this@entry=0x7ffff695f9b8, lock=...) at js/src/jsgc.cpp:3661
#6  0x00000000008f8269 in js::GCHelperState::work (this=this@entry=0x7ffff695f9b8) at js/src/jsgc.cpp:3544
#7  0x0000000000a1d4b9 in js::HelperThread::handleGCHelperWorkload (this=this@entry=0x7ffff6932800) at js/src/vm/HelperThreads.cpp:1671
#8  0x0000000000a1e261 in js::HelperThread::threadLoop (this=0x7ffff6932800) at js/src/vm/HelperThreads.cpp:1735
#9  0x0000000000acd811 in nspr::Thread::ThreadRoutine (arg=0x7ffff692e160) at js/src/vm/PosixNSPR.cpp:45
#10 0x00007ffff7bc4182 in start_thread (arg=0x7ffff5cc3700) at pthread_create.c:312
#11 0x00007ffff6cb447d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
rax	0x0	0
rbx	0x7ffff7e66000	140737352458240
rcx	0x7ffff6ca588d	140737333844109
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7ffff5cc2b40	140737317186368
rsp	0x7ffff5cc2b40	140737317186368
r8	0x7ffff5cc3700	140737317189376
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7ffff5cc2900	140737317185792
r11	0x7ffff6c27ee0	140737333329632
r12	0x7ffff695d430	140737330402352
r13	0x7ffff5cc2bc0	140737317186496
r14	0x7ffff5cc2bd0	140737317186512
r15	0x7ffff695d430	140737330402352
rip	0x8e4cc2 <js::gc::ZoneHeapThreshold::updateForRemovedArena(js::gc::GCSchedulingTunables const&)+226>
=> 0x8e4cc2 <js::gc::ZoneHeapThreshold::updateForRemovedArena(js::gc::GCSchedulingTunables const&)+226>:	movl   $0x784,0x0
   0x8e4ccd <js::gc::ZoneHeapThreshold::updateForRemovedArena(js::gc::GCSchedulingTunables const&)+237>:	callq  0x4a6f30 <abort()>
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/794a18afbb25
user:        Jon Coppeard
date:        Tue Jan 05 15:07:58 2016 +0000
summary:     Bug 1236564 - Fix various minor issues with getting/setting GC parameters r=terrence

This iteration took 295.731 seconds to run.
Jon, is bug 1236564 a likely regressor?
Blocks: 1236564
Flags: needinfo?(jcoppeard)
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attached patch bug1254108-gc-params-assertion (obsolete) — Splinter Review
The problem is that gcTriggerBytes_ is changed when some GC paramaters are set, so this assertion doesn't hold.

Now that we wait for background sweeping to finish before the end of the GC, we can recalculate the trigger threshold at this point rather than updating from the background thread as we free arenas.  This simplifies things a little and means we gcTriggerBytes_ doesn't have to be atomic.

It does mean we will end up with a slightly different value for the trigger thresholds as there will have been more allocation since the start of background sweeping.  I don't think this matters much.
Attachment #8727880 - Flags: review?(terrence)
Comment on attachment 8727880 [details] [diff] [review]
bug1254108-gc-params-assertion

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

Woot!

/me marks this off his todo list
Attachment #8727880 - Flags: review?(terrence) → review+
Backed out for performance regression on splay:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f20e4b23941
Updating the triggers at the end of the GC causes us to collect in fewer incremental slices on the splay benchmark, probably because we start collection later.  This has a large effect on the SplayLatency score.
It seems removing that code's not an option, so here's a fix.

The problem is that the assertion is not correct.  The zone trigger bytes is calculated from the base size multiplied by the growth factor, but this is then limited to tunables.gcMaxBytes() in ZoneHeapThreshold::computeZoneTriggerBytes.  If this happens then we may reduce trigger bytes to zero while finalizing on the background thread.

This is only happening because of the ridiculous value of lowFrequencyHeapGrowth set by the fuzz test.
Attachment #8729481 - Flags: review?(terrence)
Attachment #8727880 - Attachment is obsolete: true
Comment on attachment 8729481 [details] [diff] [review]
bug1254108-remove-assert

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

Yeah, I think that will also work and is certainly an improvement over what's there now.

On the other hand, could we tweak the IGC tunings to continue working well on Splay? It doesn't seem like depending on the background thread sweeping at some rate is actually more stable or correct.
Attachment #8729481 - Flags: review?(terrence) → review+
Keywords: leave-open
(In reply to Terrence Cole [:terrence] from comment #9)
> On the other hand, could we tweak the IGC tunings to continue working well
> on Splay? It doesn't seem like depending on the background thread sweeping
> at some rate is actually more stable or correct.

File bug 1257134 for followup on this approach.
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Shell only test. Patch fixes bogus assert. I don't think we should uplift this. WONTFIX 47.
Fix landed in 48, fwiw.
You need to log in before you can comment on or make changes to this bug.