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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

Comment 1

[Fuzzing Team]

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.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jon, is bug 1236564 a likely regressor?

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1254108-gc-params-assertion

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.

Comment 4

[Terrence Cole [: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

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/818e0d0ecbcc

Comment 6

[Jon Coppeard (:jonco)]

Backed out for performance regression on splay:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f20e4b23941

Comment 7

[Jon Coppeard (:jonco)]

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.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1254108-remove-assert

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.

Comment 9

[Terrence Cole [:terrence]]

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.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0097406a60

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fb0097406a60

Comment 12

[Jon Coppeard (:jonco)]

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

Comment 13

[Eric Faust [:efaust]]

Shell only test. Patch fixes bogus assert. I don't think we should uplift this. WONTFIX 47.

Comment 14

[Eric Faust [:efaust]]

Fix landed in 48, fwiw.