559321 - Test more boundary cases for int/double conversion

Status: RESOLVED FIXED

(Tamarin Graveyard :: Build Config, defect, P3)

Description

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

In http://hg.mozilla.org/tamarin-redux/rev/2938 we heavily revised how conversion between doubles and integers works.

As far as I can tell, that changeset was a response to bug 521353.  However, one change to a critical components, the CAN_BE_INT_ATOM macro, was simply wrong, and wrong in a way that automated testing should have exposed (see comments in bug 559082).

To guard against similar oversights in the future, I propose that the acceptance tests be extended to include index property lookups of common boundary cases for integer manipulation.  In particular, I propose we add tests of:

 - every case of exactly one set bit (ie 2^k, for 0 <= k < 64)
 - every case of exactly one bit unset 
 - every case of 0*1* (ie 2^k-1, for 0<=k<64)
 - every case of 1*0*

The reason to cover every case above is that we may well change the Atom representation in the near future (e.g. adding variable width type tags) and it strikes me as easier to set up the code to do them all right now.  I chose 64-bits based on my expectations about architectures we will target for the foreseeable future.  Such tests should not impose a significant computational burden to cover all of the cases above.

(I do not know enough about the test infrastructure to tell if we are exercising similar boundary cases for standard arithmetic operations; if not, then something should be done about that, although in that context it is not as easy to justify the "try 'em all" approach.)

Comment 1

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

(By "index property lookups", I really mean tests of the set[] operator, get[] operator, in (has) operator, and also the delete operator;it seems from my review of the code that each needs to be tested independently because the code is not factored in a manner that shares the relevant predicates and conversions.)

Comment 2

[Trevor Baker]

Felix, are you offering up the tests?

Comment 3

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

I was planning to work on fixing bug 559082 first, since I had already claimed that.  (I just thought I'd put this bug into the work queue while the matter was on my mind.)

But I suppose it would be good to put in the tests I am proposing here as part of fixing bug 559082.

I don't know enough about the test infrastructure yet to feel comfortable claiming this bug for myself, but I will be happy to upload code to this ticket that someone else could reformat to fit our standards.

I'll try to put something together today.

Comment 4

[Steven Johnson]

(In reply to comment #0)
> As far as I can tell, that changeset was a response to bug 521353.  However,
> one change to a critical components, the CAN_BE_INT_ATOM macro, was simply
> wrong, and wrong in a way that automated testing should have exposed (see
> comments in bug 559082).

Guilty as charged. Agreed that better test sets are overdue. I'll be happy to help massage tests into appropriate form.

Comment 5

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

Attachment: first draft exercising boundary cases of obj/arr/vector/bytearray

I think this is essentially what I was thinking of in my description, (perhaps a bit over-engineered for the task at hand).

My goal was to demonstrate its utility via bug 559082, but in fact I just hit an avm assertion failure on a 32-bit build, so I am going to look into that and see if something serious has come up, or if this is another bug that has been fixed since I last updated and built...

Comment 6

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

Attachment: first draft exercising boundary cases of obj/arr/vector/bytearray

provide a bit more info about context of failing case

Comment 7

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

This test case isn't perfect yet, because it should probably be separating
modifications to a freshly allocatoin (clean) object/array/etc from
modifications to one that has already undergone a series of modifications.  The
current test bangs on one entity repeatedly; this is indeed important to test
(see bug 559401) but so is the other case.

Comment 8

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

Attachment: second draft with fixes for bytearray idiosyncracies

This file, with my fix for bug 559401 and bug 559565 (attached to the latter), now runs to completion on a 32-bit build!

On a 64-bit build, it dies in the interpreter, but that may well be because I need to pull Steven's changes from today...

Comment 9

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

By "it dies in the interpreter", I meant to say that it dies with -Dinterp

Comment 10

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

Comment on attachment 439302 [details]
second draft with fixes for bytearray idiosyncracies

Its bigger and hairier than I hoped it would be.  If you want to retract your offer to massage it, let me know.  :)

Comment 11

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

Question: what is minimum amount of memory we have on our test machines?  I chose certain parameters based on seeing significant slowdown (e.g. trying to create and manipulate a ByteArray of length 2^29 was sluggish on my machine with 4 GB of ram), but we may want to reduce them further, perhaps to different levels depending on the host.  I do not know how to do that (yet).

Comment 12

[Lars T Hansen]

On mobile we have a few 10s of MB.  Some test cases use conditional compilation to size the tests appropriately for mobile; ping Brent / Dan for details.

Comment 13

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

Attachment: draft 3; fits test framework

draft 3; fits test framework

Also works around bytearray and vector idiosyncracies, mostly by skipping the weird cases for them.

Also tried to trim down on the number of tests executed to try to keep test running time down.  However the degree of slowdown in some cases is so extreme that it may be a symptom of a performance bug.

Comment 14

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

Comment on attachment 477754 [details] [diff] [review]
draft 3; fits test framework

Oops I accidentally uploaded a version of the patch with nearly-bogus values for bytArrBits and vectorBits.  (It was left over from when I was gathering the timings that I explicitly wrote in the comments as demonstrations that overly large values for bytArrBits are unworkable.)

Removing review requests temporarily; I'll fix, repost, and re-request reviews.

Comment 15

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

Attachment: draft 3.1; fits test framework

Fixed bytArrBits and vectorBits values to more appropriate default settings.

I'm running a sandbox build with this patch now at: http://tamarin-builds.mozilla.org/tamarin-redux/changes/2566

Comment 16

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

(In reply to comment #15)
> Created attachment 478287 [details] [diff] [review]
> draft 3.1; fits test framework
> 
> Fixed bytArrBits and vectorBits values to more appropriate default settings.
> 
> I'm running a sandbox build with this patch now at:
> http://tamarin-builds.mozilla.org/tamarin-redux/changes/2566

This sandbox build attempt failed on Android:

http://asteam.corp.adobe.com/builds/sandbox/5439-99457fa74ed4/android/acceptance-avmshell_d.log

Still trying to recreate the problem locally.  So far I have been unable to reproduce on my Nexus One.

Comment 17

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

(In reply to comment #16)
> Still trying to recreate the problem locally.  So far I have been unable to
> reproduce on my Nexus One.

I ended up having to use a Motorola Droid to reproduce the problem.

It seems like an out-of-memory condition where the operating system is SIGKILL'ing the avmshell process.

Not sure what we can/should do about this.

My short term plan is to first make the test less aggressive in its memory usage (so that I can check it in without breaking the android buildbot), and (2.) open up a separate bug to track this issue with this particular out-of-memory condition on android so that we can have a separate discussion about what to do about it.

Comment 18

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

(In reply to comment #17)
> (2.) open up a separate bug to track this issue with this particular
> out-of-memory condition on android so that we can have a separate discussion
> about what to do about it.

Filed as Bug 600939

Comment 19

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

Attachment: draft 3.2; avoids Motorola Droid OOM

This is an old test that I have finally updated to work cleanly.

Ed suggested Werner as a good candidate for review; feel free to reassign as you like.

Successful build bot run from last night:

http://tamarin-builds.mozilla.org/tamarin-redux/waterfall?last_time=1285886000&builder=windows-compile-sandbox&builder=windows64-compile-sandbox&builder=mac-intel-10.4-compile-sandbox&builder=mac-intel-10.5-compile-sandbox&builder=mac64-intel-compile-sandbox&builder=mac-ppc-10.4a-compile-sandbox&builder=mac-ppc-10.4b-compile-sandbox&builder=mac-ppc-10.5a-compile-sandbox&builder=mac-ppc-10.5b-compile-sandbox&builder=mac64-ppc-compile-sandbox&builder=mac64b-ppc-compile-sandbox&builder=linux-compile-sandbox&builder=linux2-compile-sandbox&builder=linux64-compile-sandbox&builder=winmobile-emulator-compile-sandbox&builder=solaris-sparc-compile-sandbox&builder=solaris-sparc2-compile-sandbox&builder=android-compile-sandbox&builder=linux-arm-compile-sandbox&builder=linux-arm2-compile-sandbox&builder=linux-mips-compile-sandbox&builder=linux-sh4-compile-sandbox

Comment 20

[Dan Schaffer]

Comment on attachment 480125 [details] [diff] [review]
draft 3.2; avoids Motorola Droid OOM

the testcase looks good.  On nexxus one I saw the OS kill the process when setting bytArrBits>=27.  Setting to 25 seems fine.  I ran on n1 and mac fine with hybrid and -Dinterp.  I thought the testcase was well written and commented well.

Comment 21

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

Comment on attachment 480125 [details] [diff] [review]
draft 3.2; avoids Motorola Droid OOM

pushed TR 5319:ec7b9c9a5620
  http://hg.mozilla.org/tamarin-redux/rev/ec7b9c9a5620

Comment 22

[Brent Baker]

(In reply to comment #21)
> Comment on attachment 480125 [details] [diff] [review]
> draft 3.2; avoids Motorola Droid OOM
> 
> pushed TR 5319:ec7b9c9a5620
>   http://hg.mozilla.org/tamarin-redux/rev/ec7b9c9a5620

This testcase is causing the SH4 machine to run out of memory when running with buildbot, there is just enough memory that the testcase will run fine if you run locally without buildbot running. Testcase marked as skip on sh4.* in changeset 5352:168e3635dd48