Closed Bug 931062 Opened 6 years ago Closed 6 years ago

Unit test TestPLDHash.cpp fails on Android

Categories

(Core :: XPCOM, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: dminor, Assigned: dminor)

References

Details

Attachments

(1 file, 2 obsolete files)

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).
Sounds like a regression from bug 927705, which I think added some new tests.
Blocks: 927705
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?
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.
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!
Attachment #823514 - Flags: review?(blassey.bugs)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
__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.
I'll give it a test today.
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?
couple of questions for Nicholas
Flags: needinfo?(n.nethercote)
> 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.
Flags: needinfo?(n.nethercote)
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.
__ANDROID__ seems fine, then.  How does the rest of the change look?
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.
(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.
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 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
Attachment #823514 - Flags: review?(blassey.bugs)
: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.
Flags: needinfo?(n.nethercote)
> :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.
Flags: needinfo?(n.nethercote)
Attached patch Disable one test for Android (obsolete) — Splinter Review
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.
Assignee: n.nethercote → dminor
Attachment #823514 - Attachment is obsolete: true
Attachment #825238 - Flags: review?(n.nethercote)
Attachment #825238 - Flags: review?(n.nethercote) → review+
dminor, are you happy to land this change?
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
Sorry, I'll update the patch and do a Try run this time.
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
Attachment #825238 - Attachment is obsolete: true
Attachment #826069 - Flags: review?(n.nethercote)
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.)
Attachment #826069 - Flags: review?(n.nethercote) → review+
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
Fair enough.  Thanks for the fix.
https://hg.mozilla.org/mozilla-central/rev/64d6abfbc481
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.