941649 - Multiple jit-tests fail on single core ARM target. (mostly parallel tests)

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Branislav Rankov [:rankov]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131115110311

Steps to reproduce:

On ARM target with a single core, build SpiderMonkey with following configure options:
../configure --target=arm-linux-gnueabi --enable-debug --disable-optimize --without-intl-api --disable-gold --with-system-nspr --with-nspr-prefix=....

The code was taken from mozilla-central repository.

Run the jit tests with following options
./jit-test/jit_test.py arm_build/shell/js --tbpl --write-failure-output -w test-jit.log -t 5000




Actual results:

The following tests failed:
basic/bug839215.js
basic/offThreadCompileScript.js
debug/onNewScript-off-main-thread.js
ion/bug906885.js
parallel/Array-buildPar-nested.js
parallel/Array-filterPar-truthy.js
parallel/Array-mapPar-assign-to-def-prop.js
parallel/Array-mapPar-nested.js
parallel/Array-mapPar-short.js
parallel/Array-reducePar-nested-sum-each-row.js
parallel/alloc-array-nonwritable.js
parallel/alloc-array.js
parallel/alloc-different-objs.js
parallel/alloc-many-objs.js
parallel/alloc-obj.js
parallel/alloc-too-many-objs.js
parallel/bailout-executed.js
parallel/bitops-values.js
parallel/bug853573.js
parallel/bug909743.js
parallel/closure-allocate-invoke.js
parallel/closure-creation.js
parallel/closure-eta-expand.js
parallel/closure-invoke.js
parallel/closure-nested-branch.js
parallel/closure-nested-compute.js
parallel/closure-nested.js
parallel/createthis.js
parallel/ic-setelement.js
parallel/ic-setproperty.js
parallel/reduce-higher-dim.js
parallel/scan-3.js
parallel/scatter-10.js
parallel/scatter-11.js
parallel/scatter-12.js
parallel/scatter-13.js
parallel/scatter-6.js
parallel/scatter-7.js
parallel/scatter-8.js
parallel/strings.js
parallel/write-array.js
parallel/write-obj.js



Expected results:

The tests should pass.

Comment 1

[Branislav Rankov [:rankov]]

I have investigated the cause of these failures:

basic/bug839215.js is due to low memory, so it should be ignored. It works once swap is enabled.

All the other failures are provoked by this change: https://hg.mozilla.org/mozilla-central/rev/86ffabbadeb6

If this is reverted, the tests pass.

Tests basic/offThreadCompileScript.js and debug/onNewScript-off-main-thread.js fail because there are no worker threads on which to compile. 

The rest of the tests fail because rt->workerThreadState is not initialized.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v1: init workerThreadState in ForkJoinShared

rankov: If you could give this a whirl on your platform, that would be great.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v1: assert workerThreadState inited for non JS context

My hope is that this will catch future instances of this bug.

Comment 4

[Branislav Rankov [:rankov]]

Comment on attachment 8339274 [details] [diff] [review]
patch A v1: init workerThreadState in ForkJoinShared

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

I applied the patch and tested the bugs that previously failed. All the bugs that segfaulted previously, now pass.

Only bugs that fail now are: basic/offThreadCompileScript.js and debug/onNewScript-off-main-thread.js. These are a separate issue and are reported under bug941672.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8339274 [details] [diff] [review]
patch A v1: init workerThreadState in ForkJoinShared

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

I'm not comfortable with us initializing workerThreadState here. We're don't use asynchronous worker threads -- other than getting the # of threads we should try to use here: http://dxr.mozilla.org/mozilla-central/source/js/src/vm/ThreadPool.cpp#198

The existing assumption in bhackett's code seems to be: if it's not a JSContext, it's an async worker thread. Seems like the correct fix here would be fix those places to also take into account ForkJoin as well.

Comment 6

[Shu-yu Guo [:shu]]

s/We're/We/

Comment 7

[Shu-yu Guo [:shu]]

Brian, is WorkerThreadState only meant to be used for synchronization between the async worker threads with specific workloads (parsing, Ion, etc)?

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8339278 [details] [diff] [review]
patch B v1: assert workerThreadState inited for non JS context

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

On second thought, this seems like a useful addition.

Felix, could you make this only for ExclusiveContext, not JS or ForkJoin?

Comment 9

[Shu-yu Guo [:shu]]

Attachment: Part 1: Always report number of workers as number of CPUs - 1

No reason to special case when the machine is single-cored. There'll always be a main thread, so always report num workers as num CPUs - 1.

Comment 10

[Shu-yu Guo [:shu]]

Attachment: Part 2: Fix off-main-thread alloc path in the GC

PJS hits the off-main-thread path as well, but since PJS is synchronous with the main thread, there's no need to lock or wait.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8339660 [details] [diff] [review]
Part 1: Always report number of workers as number of CPUs - 1

The reason for the original change is that, in various cases, including parallel asm.js compilation, the main thread blocks on compilation threads so we actually want, e.g., 2 helper threads on a 2 core machine.  The one-core special case is to avoid turning on background ion compilation when there is only 1 core since this measurably hurt perf.  That's kindof a hack, though, and I have a better fix in bug 941827 which hopefully would land soon.

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Part 3: Add a workerThreadCount shell testing function.

We should be using # of worker threads to test for success of OMT stuff, not whether or not the build is threadsafe.

Comment 13

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #11)
> Comment on attachment 8339660 [details] [diff] [review]
> Part 1: Always report number of workers as number of CPUs - 1
> 
> The reason for the original change is that, in various cases, including
> parallel asm.js compilation, the main thread blocks on compilation threads
> so we actually want, e.g., 2 helper threads on a 2 core machine.  The
> one-core special case is to avoid turning on background ion compilation when
> there is only 1 core since this measurably hurt perf.  That's kindof a hack,
> though, and I have a better fix in bug 941827 which hopefully would land
> soon.

I see. In that case PJS needs to do its own thing.

Comment 14

[Shu-yu Guo [:shu]]

Part 2 fixes the parallel tests, and part 3 fixes the rest.

Comment 15

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2e43ded4b791
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/de5aa094b55f

Marking as [leave open] for Felix's assert patch.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e43ded4b791
https://hg.mozilla.org/mozilla-central/rev/de5aa094b55f

Comment 17

[Branislav Rankov [:rankov]]

I run tests on my platform with the latest code from mozilla-central and all the test listed above pass now.

A side note: I cannot modify the feedback flag. Can someone else update it for me?

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v2: assert workerThreadState inited for exclusive context

Comment 19

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v3: assert workerThreadState inited for exclusive context

(added #ifdef JS_WORKER_THREADS guard)

Comment 20

[Felix S. Klock II [:pnkfelix, :fklock]]

try build for patch B v3:

  https://tbpl.mozilla.org/?tree=Try&rev=87887d8f805f

Comment 21

[Felix S. Klock II [:pnkfelix, :fklock]]

The assert landed in:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/17aacd34982c

thus removing [leave open] mark.

Comment 22

[Andrew McCreight [:mccr8]]

You need to manually close the bug, because the scripts that update bugs to closed only run when patches are merged to mozilla-central.