931062 - Unit test TestPLDHash.cpp fails on Android

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Dan Minor [:dminor]]

This test fails with return code 9 when run from the test package on Android.

Last good revision on Cedar was https://hg.mozilla.org/projects/cedar/rev/7854fd34179d (Oct 22, 2013) and failed on the next merge of m-c to Cedar https://hg.mozilla.org/projects/cedar/rev/bc07f6ca375c (Oct 23, 2013).

Comment 1

[Andrew McCreight [:mccr8]]

Sounds like a regression from bug 927705, which I think added some new tests.

Comment 2

[Nicholas Nethercote [inactive]]

Yeah, bug 927705 added that test.  Among other things, it allocates a 0.5GB array;  there's a good chance that is what's failing (http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestPLDHash.cpp#17).

Are you able to catch the problem in a debugger?

Comment 3

[Dan Minor [:dminor]]

Return code 9 indicates the test is being sent SIGKILL, which usually means that the test is the victem of the OOM killer.

The test that seems to be the problem is test_pldhash_grow_to_max_capacity() (http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestPLDHash.cpp#74). The tests run successfully if it is commented out, and fail when it is run by itself.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Skip one test in TestPLDHash.cpp on Android because it tends to OOM.

dminor, can you please test this patch?

blassey, I'm not sure if |__ANDROID__| is the best way to detect Android --
|ANDROID| is also possible, AFAICT.  mfinkle suggested that you might know.
Thanks!

Comment 5

[Nathan Froyd [:froydnj]]

__ANDROID__ is defined by my NDK's compiler; ANDROID isn't (it might come from system headers somehow).  __ANDROID__ is preferred because it's in the compiler's implementation namespace, where people shouldn't be just randomly adding things.

Comment 6

[Dan Minor [:dminor]]

I'll give it a test today.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 823514 [details] [diff] [review]
Skip one test in TestPLDHash.cpp on Android because it tends to OOM.

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

::: xpcom/tests/TestPLDHash.cpp
@@ +17,5 @@
>  static bool test_pldhash_Init_capacity_ok()
>  {
>    // Try the largest allowed capacity.  With PL_DHASH_MAX_SIZE==1<<26, this
>    // will allocate 0.5GB of entry store on 32-bit platforms and 1GB on 64-bit
> +  // platforms.  On Android, this is unreliable so we skip this test.

out of curiosity, would it be reasonable to allocate something smaller on Android and B2G? Maybe 128Mb?

@@ +19,5 @@
>    // Try the largest allowed capacity.  With PL_DHASH_MAX_SIZE==1<<26, this
>    // will allocate 0.5GB of entry store on 32-bit platforms and 1GB on 64-bit
> +  // platforms.  On Android, this is unreliable so we skip this test.
> +  bool ok = true;
> +#ifndef __ANDROID__

we tend to use ANDROID instead of __ANDROID__. Keep in mind that this will disable it for B2G as well. Is that what you want?

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

couple of questions for Nicholas

Comment 9

[Nicholas Nethercote [inactive]]

> out of curiosity, would it be reasonable to allocate something smaller on
> Android and B2G? Maybe 128Mb?

Not really.  It's dictated by the pldhash's max capacity, which is (somewhat arbitrarily) 64M.  (Each entry is a minimum of 8 bytes, so 8*64M = 512M.)  In other bugs people have been asking why the max capacity isn't higher, and testability was one of the reasons :)

pldhash is platform-independent code so I'm not worried about omitting Android for this test.


> > +#ifndef __ANDROID__
> 
> we tend to use ANDROID instead of __ANDROID__. Keep in mind that this will
> disable it for B2G as well. Is that what you want?

Since the failures haven't been seen on B2G yet, sticking with __ANDROID__ seems best -- i.e. disable the test on fewer configurations.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

both ANDROID and __ANDROID__ will disable the test on B2G. If you don't want to disable for B2G you'll need to use MOZ_WIDGET_ANDROID.

Comment 11

[Nicholas Nethercote [inactive]]

__ANDROID__ seems fine, then.  How does the rest of the change look?

Comment 12

[Dan Minor [:dminor]]

It is actually a different test which is causing it to be OOM killed: http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestPLDHash.cpp#74, so the patch as written doesn't fix the problem.

If this is expected to happen based upon recent changes then we can just #ifdef it out. Otherwise, I guess we'll need to do some more investigation.

Comment 13

[(not currently active) Ted Mielczarek]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Since the failures haven't been seen on B2G yet, sticking with __ANDROID__
> seems best -- i.e. disable the test on fewer configurations.

dminor would know for sure, but I think this is just because we aren't running these tests on B2G yet. With his recent work we've started running them on Android on certain project branches, which is why you haven't seen this elsewhere.

Comment 14

[Dan Minor [:dminor]]

As far as I know these tests have only been run previously as part of 'make check' for desktop builds. I've been working to get these running from the test package, which allows us to run them for non-desktop builds, but only on Android 4.0 (pandaboards) at the moment.

I would guess that the B2G emulator would run into similar problems, but it makes sense to me to only disable for Android for now.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 823514 [details] [diff] [review]
Skip one test in TestPLDHash.cpp on Android because it tends to OOM.

clearing the review based on comment 12

Comment 16

[Dan Minor [:dminor]]

:njn are you comfortable with disabling the test_pldhash_grow_to_max_capacity() test on Android? I can prepare a patch for that.

Otherwise, I'll plan to add the entire test to skipped test manifest for Android cpp unittests until this bug is fixed. Currently, the cpp unittests are only being run for Android on Cedar (they don't run as part of make check.) Skipping this test should get them green, which means we can start running them on inbound and m-c, which is good from a test coverage point of view.

Comment 17

[Nicholas Nethercote [inactive]]

> :njn are you comfortable with disabling the
> test_pldhash_grow_to_max_capacity() test on Android? I can prepare a patch
> for that.

Yes please!  I was going to do that today, but since you're in a better position to test it, it would be great if you could do it.  I can review it.

Comment 18

[Dan Minor [:dminor]]

Attachment: Disable one test for Android

I used MOZ_WIDGET_ANDROID so this will only be disabled for Android. We can always switch to ANDROID later on if we need to disable this on B2G as well.

Comment 19

[Nicholas Nethercote [inactive]]

dminor, are you happy to land this change?

Comment 20

[Dan Minor [:dminor]]

Thank for the review, https://hg.mozilla.org/integration/mozilla-inbound/rev/38a50c61c285

Comment 21

[Ed Morley [:emorley]]

Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29970836&tree=Mozilla-Inbound

Yey warnings as errors. I'd say fix in place, but tree pretty poorly again today, so would rather speed up getting to green first.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e30094c62783

Comment 22

[Dan Minor [:dminor]]

Sorry, I'll update the patch and do a Try run this time.

Comment 23

[Dan Minor [:dminor]]

Attachment: Disable one test for Android

Not sure if this really needs another review. I've added an #ifdef block to remove the funtions associated with the test to silence the warning.

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=352075210617

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 826069 [details] [diff] [review]
Disable one test for Android

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

At this point I would actually change it so the test is always run, but on Android it just immediately returns true.  That way you don't need two separate #ifdef blocks.  But I'm ok with how you've done it as well, if you can't be bothered changing.  (If you do change to the single block, no need to re-review.)

Comment 25

[Dan Minor [:dminor]]

Thanks for the review. Unfortunately, I would have to change both the hash function and the test itself to avoid getting another warning about unused code, which seemed as messy as the two #ifdefs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/64d6abfbc481

Comment 26

[Nicholas Nethercote [inactive]]

Fair enough.  Thanks for the fix.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64d6abfbc481