Closed Bug 559321 Opened 10 years ago Closed 9 years ago

Test more boundary cases for int/double conversion

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
Q3 11 - Serrano

People

(Reporter: pnkfelix, Assigned: pnkfelix)

References

Details

Attachments

(2 files, 4 obsolete files)

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.)
(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.)
Felix, are you offering up the tests?
Depends on: 521353
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.
Blocks: 559082
(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.
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...
provide a bit more info about context of failing case
Attachment #439044 - Attachment is obsolete: true
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.
Depends on: 559401
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...
Attachment #439062 - Attachment is obsolete: true
By "it dies in the interpreter", I meant to say that it dies with -Dinterp
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.  :)
Attachment #439302 - Flags: feedback?(stejohns)
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2
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).
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.
No longer blocks: 559082
Depends on: 559082
Assignee: nobody → fklockii
Status: NEW → ASSIGNED
Attached patch draft 3; fits test framework (obsolete) — Splinter Review
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.
Attachment #477754 - Flags: review?(dschaffe)
Attachment #477754 - Flags: feedback?(treilly)
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.
Attachment #477754 - Flags: review?(dschaffe)
Attachment #477754 - Flags: feedback?(treilly)
Attached patch draft 3.1; fits test framework (obsolete) — Splinter Review
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
Attachment #477754 - Attachment is obsolete: true
(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.
(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.
(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
Attachment #480125 - Flags: review?(wsharp) → review+
Attachment #439302 - Flags: feedback?(stejohns) → feedback+
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.
Attachment #480125 - Flags: superreview?(dschaffe) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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
You need to log in before you can comment on or make changes to this bug.