931882 - Jit-test test basic/bug908915.js times out on Android

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dan Minor [:dminor]]

As can be seen here [1], the basic/bug908914.js test is consistently timing out on Android. 

Unfortunately, the test appears to be retried automatically, causing a large number of failed test runs to occur.

[1] https://tbpl.mozilla.org/?tree=Cedar&rev=2d3ca4d9d3b9

Comment 1

[Terrence Cole [:terrence]]

Also testBullet.js and testCaching.js appear to perma-fail, probably because we can't assume a filesystem on android.

Marty, are these tests expected to fail on ARM?

Comment 2

[Luke Wagner [:luke]]

I can test in a browser (with bug 929236) and caching does work on ARM, so the failures are probably specific to the shell setup.  From the failures, I'm guessing some of the filesystem I/O operations in ShellOpenAsmJSCacheEntryFor(Read|Write) (js/src/shell/js.cpp) are failing which cause the cache entry not to be written.  If anyone can run this locally: perhaps you could put in some printfs in these functions to see which call is failing?

Note: the cache file directory is set by --js-cache and this is js/src/jit-test/.js-cache for jit-tests.py (see JS_CACHE_DIR in js/src/tests/lib/jitttest.py).  Furthermore, in multi-process builds, jit-tests.py passes --js-cache-per-process which makes a new subdir under the .js-cache for each process so that parallel tests don't clobber each other's cache files.

Comment 3

[Dan Minor [:dminor]]

The testBullet.js and testCaching.js failures reproduce for me on my pandaboard, so I'll have a look. Unfortunately basic/bug908914.js works locally.

Comment 4

[Dan Minor [:dminor]]

Filed bug 932940 to fix the --js-cache problem for Android.

Comment 5

[Terrence Cole [:terrence]]

Was this fixed by bug 932940?

Comment 6

[Phil Ringnalda (:philor)]

No, it remains unfixed.

Comment 7

[Dan Minor [:dminor]]

Can we disable this test in the meantime?

If this stabilizes things on Cedar, then we can enable jit-tests on Try for Android which would make it easier to investigate this problem for people without pandaboards.

Comment 8

[Terrence Cole [:terrence]]

Yes, I think it makes sense to at least get the tests that do currently work running asap. Please file a follow-up bug and CC myself and luke.

Comment 9

[Douglas Crosher [:dougc]]

(In reply to Luke Wagner [:luke] from comment #2)
> I can test in a browser (with bug 929236) and caching does work on ARM, so
> the failures are probably specific to the shell setup.  From the failures,
> I'm guessing some of the filesystem I/O operations in
> ShellOpenAsmJSCacheEntryFor(Read|Write) (js/src/shell/js.cpp) are failing
> which cause the cache entry not to be written.  If anyone can run this
> locally: perhaps you could put in some printfs in these functions to see
> which call is failing?

ShellOpenAsmJSCacheEntryForWrite
 jsCacheAsmJSPath: /data/local/tmp/tests/jit-tests/jit-tests/.js-cache/27388/asmjs.cache
 jsCacheDir: /data/local/tmp/tests/jit-tests/jit-tests/.js-cache/27388

If the .js-cache directory does not already exist then mkdir .../.js-cache/27388 fails.

If the .js-cache directory is manually created before running the test then is succeeds.

Should jit-test create the .js-cache directory, or should the js shell? 

Also noticed while testing that nestedShell("--js-cache", ...) tries to pass a directory even if none was defined and it passes '(null)'.  Perhaps it could be guarded:

NestedShell(JSContext *cx, unsigned argc, jsval *vp)
{
...
        if (!strcmp(argv.back(), "--js-cache") && jsCacheDir) {

Comment 10

[Luke Wagner [:luke]]

(In reply to Douglas Crosher [:dougc] from comment #9)
> If the .js-cache directory is manually created before running the test then
> is succeeds.
> 
> Should jit-test create the .js-cache directory, or should the js shell? 

The jit-test harness should be creating the .js-cache directory.  I *thought* that's what the patch in bug 932940 was doing.  The relevant line is the dm.mkDirs(Test.CacheDir) in run_tests_remote in jittests.py

>         if (!strcmp(argv.back(), "--js-cache") && jsCacheDir) {

Sounds good

Comment 11

[Dan Minor [:dminor]]

Bug 932940 sets up the .js-cache directory on the remote device and I also changed the mozharness script to disable parallel execution for ARM. This fixed the testBullet.js and testCaching.js tests that Terrence mentioned which led me to believe the timeout was not related to caching issues.

Comment 12

[Luke Wagner [:luke]]

Hmm, it shouldn't have been necessary to disable parallel execution as long as the --js-cache-per-process flag was also set (which it is for non-remote jit_tests).  This causes every test to get its own caching subdir so they don't clobber each other and invalidate the "is cached" assertions.

Comment 13

[Dan Minor [:dminor]]

(In reply to Luke Wagner [:luke] from comment #12)
> Hmm, it shouldn't have been necessary to disable parallel execution as long
> as the --js-cache-per-process flag was also set (which it is for non-remote
> jit_tests).  This causes every test to get its own caching subdir so they
> don't clobber each other and invalidate the "is cached" assertions.

We weren't running the remote tests in parallel anyway, I just made this explicit so as to avoid having to create additional .js-cache dirs on the device.

Comment 14

[Luke Wagner [:luke]]

(In reply to Dan Minor [:dminor] from comment #13)
> We weren't running the remote tests in parallel anyway, I just made this
> explicit so as to avoid having to create additional .js-cache dirs on the
> device.

If you weren't running them already, that's fine, but note that --js-cache-per-process causes the process to delete its own dir at the end of running so it doesn't really create that many dirs.

Comment 15

[Douglas Crosher [:dougc]]

Attachment: Correct creation jit-test remote js-cache directory

Seems that mkDirs does not create the last element of the path, so .js-cache was not being created.

Comment 16

[Douglas Crosher [:dougc]]

Attachment: Guard against passing a null js-cache in NestedShell

Comment 17

[Dan Minor [:dminor]]

Comment on attachment 832632 [details] [diff] [review]
Correct creation jit-test remote js-cache directory

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

Thanks!

Comment 18

[Douglas Crosher [:dougc]]

Attachment: Avoid a invoking a nested shell

Test basic/bug908915.js is invoking a nested shell and this continues to cause jit-test timeouts on Android remote testing.  It would appear appropriate to simply avoid invoking a nested shell in this test.

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 8334467 [details] [diff] [review]
Avoid a invoking a nested shell

This is a funny test.  Thanks Douglas!

Comment 20

[Douglas Crosher [:dougc]]

Attachment: correct the creation of the jit-test remote js-cache directory

Fix bug description. Carrying forward r+.

Comment 21

[Douglas Crosher [:dougc]]

Attachment: guard against passing a null js-cache in NestedShell

Fix bug description, and rebase.  Carrying forward r+.

Comment 22

[Douglas Crosher [:dougc]]

Attachment: avoid invoking a nested shell when running jit-test bug710947.js

Fix bug description. Carrying forward r+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc99fa6a365
https://hg.mozilla.org/integration/mozilla-inbound/rev/53d21900b302
https://hg.mozilla.org/integration/mozilla-inbound/rev/768f6bfdce7d

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fc99fa6a365
https://hg.mozilla.org/mozilla-central/rev/53d21900b302
https://hg.mozilla.org/mozilla-central/rev/768f6bfdce7d

Comment 25

[Dan Minor [:dminor]]

Thanks for looking at this! That fixed the test (and infinite retries on Cedar.)