Closed Bug 559321 Opened 10 years ago Closed 9 years ago
Test more boundary cases for int/double conversion
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.
(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.
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)
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.
Assignee: nobody → fklockii
Status: NEW → ASSIGNED
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 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.
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
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 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.