The "dense" rewrite of Array tightened up some oddities in subclasses of Arrays, but existing code turns out to depend on some of these. Consider: class ButtonGroup extends Array {} var b:ButtonGroup = new ButtonGroup(); b.push(0); since "dynamic" isn't inherited, ButtonGroup is sealed, and thus push() should throw a WriteSealed error (and now does). However, it turns out that the old version of AS3_push() didn't check for this, and quietly allowed this code to work. We need to audit the old code for "holes" of this sort and allow version-checked behavior to continue to work.

Root cause is native Array methods which don't check traits()->needsHashtable(); looking over old code, the possible culprits seem to be: ArrayObject::nextName ArrayObject::nextValue ArrayObject::nextNameIndex ArrayObject::AS3_pop ArrayObject::AS3_push ArrayObject::AS3_unshift as well as many various code paths thru the functions in ArrayClass (eg concat, reverse, etc)

This looks like it's going to be pretty painful to fix: in the old code, a smattering of functions happen to work; the structure of the "dense" array work makes it hard to support this mixed use case. (It's a must-fix, but still going to be painful.)

The specific methods that previously worked on nondynamic subclasses of Array (but shouldn't): push, unshift, concat, reverse, shift, pop, and splice (iff delCount==0).

Also: get_length returns the correct value, but set_length is ignored.

For the functions that have divergent behavior, this validates expected behavior. Set "old_player=true" if running in a pre-dense-Array build of Tamarin; set "old_player=false" for current builds. The test uses the term "mixed" to denote a sealed subclass of Array that nevertheless has some dynamic behavior due to legacy bugs.

I have a rough prototype patch that may be an adequate fix; it's mighty skanky but may be the best we can do. More info to come...

This is rather skanky, but it works. First off, examine the testcase, which exhaustively documents all existing behavior, both for dynamic (aka normal) Arrays, "true" Sealed Arrays, and the "semisealed" Arrays that existed in prior versions. The basic approach I've taken is to sniff for a sealed-Array-subclass when a ClassClosure is inited, and if we find one, swap in the new "SemiSealedArrayObject" class, which overrides just enough stuff to match the previous behavior. It's worth noting that the class is still in fact "sealed" -- has no hashtable -- the trick is that we allow its Dense Array Area to be populated; it turns out that the expected legacy behavior of SemiSealed arrays happens to be limited to operations that always result in arrays with no holes.

Comment on attachment 530453 [details] [diff] [review] Patch retargeting review

Comment on attachment 530453 [details] [diff] [review] Patch Review of attachment 530453 [details] [diff] [review]: ----------------------------------------------------------------- R+, with the test case extended to cover nontrivial inheritance chains. The coding details look OK, but there are some gaps in my understanding of the big picture. I have relied considerably on your testing to show that the correct semantics are implemented. There should be tests for the following two cases: dynamic class DynamicSealedArray extends SealedArray {} class SealedDynamicArray extends DynamicArray {} Minor possible improvements: 1) Function ArrayObject::getUintPropertyImpl() is now only called once, from ArrayObject::getUintProperty(), so it no longer needs to be broken out as a shared inline. 2) SemiSealedArrayObject::getUintProperty() calls SemiSealedArrayObject::getAtomProperty(), but this is overridden in SemiSealedArrayObject with a function that just delegates to getAtomPropertyFromProtoChain(). Unless this is intended as a hook for future extension, e.g., we are anticipating future subclasses of SemiSealedArrayObject, getUintProperty() could just call getAtomPropertyFromProtoChain() directly.

(In reply to comment #9) > There should be tests for the following two cases: > > dynamic class DynamicSealedArray extends SealedArray {} > class SealedDynamicArray extends DynamicArray {} Good catch: adding these produces assertions. Investigating.

Yep: ClassClosure::calcCreateInstanceProc needs to check for SemiSealedArrayObject::createInstanceProc and avoid propagating it, to handle the perverse case above. New patch on the way.

Comment on attachment 532700 [details] [diff] [review] Patch v2 Review of attachment 532700 [details] [diff] [review]: ----------------------------------------------------------------- R+, but please remove the following line of diagnostic code that appears to have been left in place: core()->console<<"create "<<traits()<<"\n";

TR 6304:d68818a35c28

changeset: 6305:3261ae1c14e8 user: Steven Johnson <stejohns@adobe.com> summary: Bug 654807 followup: remove extra name qualification (r=me) http://hg.mozilla.org/tamarin-redux/rev/3261ae1c14e8

tamarin-redux-serrano brbaker$ hg log -r 0a532531eb69 changeset: 6245:0a532531eb69 user: Steven Johnson <stejohns@adobe.com> date: Wed May 25 15:04:45 2011 -0700 summary: Bug 654807 - nondynamic subclasses of Array are still sort-of dynamic (r=wmaddox)

Brent, can you confirm the performance impact of these changes on the Scimark tests, specifically Fast Fourier Transform, Jacobi SOR, Monte Carlo Integration, Spares Matrix Multiply, and Factorization. This is to confirm the findings in bug http://watsonexp.corp.adobe.com/#bug=2884308.

% change on average for each scimark test betwen builds 6244:39288a7be796 and 6245:0a532531eb69 (running 10 iternations of the testcase) (m=mac, w=windows) 32m 64m 32w 64w FFT -2.9 -1.6 -1.9 -0.7 LU 0.0 -0.7 0.4 0.1 MC -2.6 -9.2 -0.1 -1.8 SOR -2.1 -2.3 -4.6 -2.5 SMM -2.8 -15.1 -11.7 -4.3 This change did cause a slow down on most of the scimark testcases. The scale of change for the SparseCompRow (Sparse Matrix Multiply) is a little exagerated since the timing on this test case in <100ms and therefore a change of a couple of milliseconds can appear drastic (65ms vs 74ms) compiled using xplat build script (../configure.py --target=<cpu>-<platform>) Raw Data: avm: ~/hg/builds/6244-39288a7be796/mac/avmshell version: 6244:39288a7be796 avm2: ~/hg/builds/6245-0a532531eb69/mac/avmshell version: 6245:0a532531eb69 avm avm2 test best avg best avg %dBst %dAvg Metric: time Dir: scimark/ 32bit mac FFT 1845 1902.3 1901 1957.3 -3.0 -2.9 LU 2410 2460.4 2407 2460.6 0.1 -0.0 MonteCarlo 2185 2267.6 2243 2325.6 -2.7 -2.6 SOR 2113 2162.7 2153 2208.1 -1.9 -2.1 SparseCompRow 80 81.6 83 83.9 -3.8 -2.8 64bit mac FFT 1294 1354 1333 1375.5 -3.0 -1.6 LU 1961 2001.6 1975 2015.9 -0.7 -0.7 MonteCarlo 1201 1227.5 1302 1339.9 -8.4 -9.2 - SOR 1464 1533.2 1512 1567.8 -3.3 -2.3 SparseCompRow 63 64.7 72 74.5 -14.3 -15.1 -- 32bit win FFT 2434.0 2479.6 2495.0 2526.1 -2.5 -1.9 LU 2767.0 2782.5 2753.0 2770.2 0.5 0.4 MonteCarlo 2721.0 2730.0 2721.0 2732.7 0.0 -0.1 SOR 2855.0 2863.6 2985.0 2994.9 -4.6 -4.6 - SparseCompRow 85.0 88.8 96.0 99.2 -12.9 -11.7 -- 64bit win FFT 1661.0 1692.4 1667.0 1703.9 -0.4 -0.7 LU 2400.0 2445.2 2405.0 2443.4 -0.2 0.1 MonteCarlo 1539.0 1569.9 1578.0 1597.6 -2.5 -1.8 - SOR 1693.0 1706.1 1727.0 1748.9 -2.0 -2.5 - SparseCompRow 82.0 83.0 85.0 86.6 -3.7 -4.3 -