956434 - Intermittent TEST-UNEXPECTED-FAIL | testArrayBufferSlice.js | --no-baseline --no-ion | 8:12 Error: Assertion failed: got 16, expected 43

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

https://tbpl.mozilla.org/php/getParsedLog.php?id=32534074&tree=Mozilla-Inbound
slave: bld-linux64-ec2-123


TEST-PASS | js/src/jit-test/tests/basic/testArgumentsPropLookup.js | --no-baseline --no-ion --no-ti
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | 
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --ion-eager --ion-parallel-compile=off
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --ion-eager --ion-parallel-compile=off --ion-check-range-analysis --no-sse3
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --baseline-eager
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --baseline-eager --no-ti --no-fpu
FAIL - basic/testArrayBufferSlice.js
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --no-baseline --no-ion
INFO exit-status     : 3
INFO timed-out       : False
INFO stdout          > 
INFO stderr         2> /builds/slave/m-in_l64-d_sm-ra-0000000000000/src/js/src/jit-test/tests/basic/testArrayBufferSlice.js:8:12 Error: Assertion failed: got 16, expected 43
INFO stderr         2> 
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --no-baseline --no-ion --no-ti
TEST-PASS | js/src/jit-test/tests/basic/testArrayComp1.js |

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Disabled for now due to this test causing other unpredictable failures on rootanalysis builds from unrelated changes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/658e342f400c

Comment 2

[Legacy TBPL/Treeherder Robot]

jandem
https://tbpl.mozilla.org/php/getParsedLog.php?id=32585370&tree=Try
Linux x86-64 try leak test spidermonkey_try-rootanalysis build on 2014-01-06 08:11:54
revision: 5093a93d0b37
slave: try-linux64-spot-425

TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --no-baseline --no-ion
INFO stderr         2> /builds/slave/try_l64-d_sm-ra-00000000000000/src/js/src/jit-test/tests/basic/testArrayBufferSlice.js:8:12 Error: Assertion failed: got 16, expected 43
make: *** [check-jit-test] Error 2

Comment 3

[Jan de Mooij [:jandem]]

Making this block GGC so that we don't forget about this. Steve is investigating.

Comment 4

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

This is a very real and major problem (when exact rooting is enabled.) I'm not really sure why this is so intermittent -- this ought to hit when you get a minor collection while creating any of the slices. I have a fix, but I'm going to try to figure out how to make it fail a little more reliably first.

Comment 5

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

The bug is that when slicing an array, a pointer to the origin buffer's contents is passed on the stack across a GC, so when that pointer is used to initialize the contents of the newly created buffer, it can be invalid.

But this only happens if the origin buffer is moved from the nursery to the tenured area when creating a slice. With JS_GC_ZEAL=7, there are lots of extra minor GCs, and if one of them hits for any other allocation, the origin buffer will get tenured and the problem won't be seen. (So sadly, forcing verification GCs too frequently  will make this sort of problem *less* likely to be found.)

I attempted to at least add some additional stuff to the test to make it fail reliably, but even a schedulegc(1) won't do it, because apparently the gczeal frequency doesn't apply to object cache allocations.

If I force it to apply to those too, I can force a failure.

Comment 6

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

Attachment: Do not pass an internal pointer on the stack

Comment 7

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

Attachment: Apply gczeal to object cache allocations

This makes the test reliably reproduce. The part I tacked onto the end of the test isn't strictly necessary, since it'll hit earlier.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8356288 [details] [diff] [review]
Apply gczeal to object cache allocations

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

r=me

::: js/src/vm/Runtime-inl.h
@@ +55,5 @@
>      if (type->shouldPreTenure())
>          heap = gc::TenuredHeap;
>  
> +    if (cx->runtime()->upcomingZealousGC())
> +        return nullptr;

Nice! Might want to make sure the test suite actually runs with this change ;-).

Comment 9

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

(In reply to Terrence Cole [:terrence] from comment #8)
> Nice! Might want to make sure the test suite actually runs with this change
> ;-).

Er, yeah. Good point. https://tbpl.mozilla.org/?tree=Try&rev=686a67373e4c

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8356277 [details] [diff] [review]
Do not pass an internal pointer on the stack

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

I think this is clearer to boot. r=me

::: js/src/vm/TypedArrayObject.cpp
@@ -621,2 @@
>  {
> -    SkipRoot skip(cx, &contents);

e_e

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/658e342f400c

Comment 12

[Jan de Mooij [:jandem]]

Please also remove the quit(0) from the test :)

Comment 13

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

Attachment: Do not pass an internal pointer on the stack

Requesting re-review for the following changes:

1. I introduced a rooting hazard. Or rather, I previously hid a rooting hazard behind an internal pointer, and this exposed it. So I Handle-ified createSlice.

2. The memset(data, 0, len) got lost, so I wasn't clearing ArrayBuffer memory when allocating without initial data.

(I'll land a test for these problems too.)

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 8356670 [details] [diff] [review]
Do not pass an internal pointer on the stack

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

r=me

::: js/src/vm/TypedArrayObject.cpp
@@ +611,5 @@
>      SetViewList(this, view);
>  }
>  
>  JSObject *
> +ArrayBufferObject::create(JSContext *cx, uint32_t nbytes, bool clear)

I generally like to add a note that the parameter is defaulted and what the default is: e.g. (..., bool clear /* = true */). I've frequently found these annotations helpful when reading as it keeps you from having to flip to the header to get that info.

::: js/src/vm/TypedArrayObject.h
@@ -67,2 @@
>  
> -    static JSObject *createSlice(JSContext *cx, ArrayBufferObject &arrayBuffer,

I wonder if this was reported as an unsafe reference?

Comment 15

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/a443aaba92c0
http://hg.mozilla.org/integration/mozilla-inbound/rev/0f5f6f400d97

Comment 16

[Hannes Verschore [:h4writer]]

I see a 4% decrease in octane-zlib. I assume it is caused by one of these patches.
http://arewefastyet.com/?a=b&view=regress#machine=11&view=breakdown&suite=octane

Regression log:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5469c3c3506d&tochange=61ba6198da7c

Comment 17

[Guilherme Lima]

But it improved octane-zlib on the 64-bit machine.
It also regressed octane-Gameboy (4% on 32-bit and 2,5% on 64-bit).
On the 32-bit it regressed asmjs-apps-zlib-throughput and asmjs-apps-zlib-loadtime.
On the 64-bit it improved asmjs-apps-zlib-throughput.
It regressed asmjs-apps-lua_binarytrees-loadtime on both machines.
It regressed asmjs-apps-box2d-loadtime on the 32-bit machine.
It regressed misc-bugs-636096-model2d on the 64-bit machine.
It regressed kraken-stringify-tinderbox on the 64-bit machine.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f5f6f400d97
https://hg.mozilla.org/mozilla-central/rev/a443aaba92c0

Comment 19

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

Attachment: Avoid unnecessary zeroing of memory

It turns out that the slowdown is due to a redundant zeroing of out-of-line memory (which is already allocated via calloc.)

Comment 20

[Terrence Cole [:terrence]]

Comment on attachment 8357595 [details] [diff] [review]
Avoid unnecessary zeroing of memory

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

Makes sense. r=me

Comment 21

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

perf fix - https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff6aa4c9bc9

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/9ff6aa4c9bc9