679102 - Rollback dense-array changes

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Library, defect)

Description

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

In Bug 610063, steven ported a change in the array representation.

Subsequently, a number of issues were uncovered.  Some were individually patched or partially addressed, but not all of the problems have been resolved.

  Bug 630945
  Bug 636535
  Bug 643009
  Bug 654807
  Bug 658677
  Bug 677923

At this point, we are probably too close to the Serrano release to actually make the dense-array change work in a backward-compatible manner via version-checks.

So the short term plan is to revert the dense-array changes and evaluate whether the performance impact is acceptable.  If so, then we will revert changes and then subsequently see if there is a way we can land a version-checked form of the feature.

Comment 1

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

(See in particular Bug 658677, comment 52, for a concise argument for reversion.)

Comment 2

[Lars T Hansen]

So are you going to rollback just the Serrano/Anza branch, or TR as well?  I ask because the work I've been doing on bug #678952, bug #678969, and probably others probably will be affected if you rollback TR.

Comment 3

[Lars T Hansen]

(In reply to Lars T Hansen from comment #2)
> So are you going to rollback just the Serrano/Anza branch, or TR as well?  I
> ask because the work I've been doing on bug #678952, bug #678969, and
> probably others probably will be affected if you rollback TR.

Nevermind, those are about vector, and this is array.  The interactions should not be a problem.

Comment 4

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

(In reply to Lars T Hansen from comment #2)
> So are you going to rollback just the Serrano/Anza branch, or TR as well?

Just in case any one else is wondering: Yes, I think rolling back in both Serrano/Anza and in TR would be best.  Otherwise I'd worry about introducing a split into three or more behaviors rather than trying to keep it contained to at most two behaviors (the old crazy one, and the new hypothetical probably ecma-compliant one).

Comment 5

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

Attachment: patchA v1: known rough cut at rollback

I took an even rougher patch and trimmed it down a bit to this.  But there's still some bits that I need to double-check or revise (e.g. the AOT reversions are probably inapprorpriate).

But I *must* go now to dinner or I will suffer consequences.  So only asking for F? rather than R?.

Comment 6

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

(In reply to Felix S Klock II from comment #5)
> Created attachment 553619 [details] [diff] [review]
> patchA v1: known rough cut at rollback
> 
> I took an even rougher patch and trimmed it down a bit to this.  But there's
> still some bits that I need to double-check or revise (e.g. the AOT
> reversions are probably inapprorpriate).
> 
> But I *must* go now to dinner or I will suffer consequences.  So only asking
> for F? rather than R?.

FYI this was based against TR-serrano  6350:fc1f3cc303b5

Comment 7

[William Maddox]

Attachment: Another try at revering the problematic dense array code

I attempted to produce this patch in a disciplined way, selectively applying either forward diffs from revision 5711 (just before the TT import) or backward diffs from revision 6351 (tip), generating each diff from via 'hg diff' on the individual commits.  I only edited the code manually to fix a few cases in which systematic changes made later needed to be applied to reverted code.  I got pretty far along, in that most of the result compiles, but it eventually bombs in builtin.cpp, generated code, due to an attempt to call a protected constructor.  Also, eyeballing the result, it appears that I failed to apply revsion 6241 to ArrayClass.cpp, and looking at my notes, I don't quite understand how that happened.  The result I ended up with is different than Felix's.  For one thing, I was willing to undo various bits of incidental refactoring and cleanup that piggybacked on the reverted changes.  But the point of the exercise was to see if I could get to something independently that I could diff against Felix's patch to determine if it was correct.

Comment 8

[William Maddox]

Comment on attachment 553619 [details] [diff] [review]
patchA v1: known rough cut at rollback

F-

On inspection, it looks reasonable, but I get the willies trying to disentangle the unrelated changes that were made to the array code before it was reverted. There are clearly some mechanical cases where it was necessary to update bits of the reverted code to conform to later fixes, and I fear we have missed something.  I attempted to produce another patch independently and systematically for comparison, and flubbed it.  I really have my doubts that this patch can be sensibly reviewed, at least as a simple reversion, that is, without understanding the intent of the multitude of interleaved and unreverted patches at more than a superficial level.  This is new code, that needs to be treated as such, not waved through with a cursory acceptance test run and crammed into a release at the last minute.  I'll take another look in the morning with fresh eyes.

Comment 9

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

(In reply to William Maddox from comment #8)
> I'll take another look in the morning with fresh eyes.

ping me before you spent time on that.

I think I can do a lot better job by focusing in on what we're concerned about (namely the length property), like you mentioned last night.

Comment 10

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

(In reply to Felix S Klock II from comment #9)
> (In reply to William Maddox from comment #8)
> > I'll take another look in the morning with fresh eyes.
> 
> ping me before you spent time on that.
> 
> I think I can do a lot better job by focusing in on what we're concerned
> about (namely the length property), like you mentioned last night.

(plus the patch would definitely regress some bugs; I reverted some changesets when making it that I should not have.  still fixing.)

Comment 11

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

Attachment: patchA v3: refined but not done

not even asking for F? on this one, because its not done yet.

(I've been revising the reverted code to try to narrow in on the specific implementation changes that causes .length semantic changes... but this is *in-progress*, and *must not land*.)

Comment 12

[William Maddox]

Attachment: Buildable, mostly working reversion patch

This patch was derived almost entirely by systematic application of diffs, but I had to make the ArrayObject() constructor public, to avoid a problem compiling builtin.cpp.  Looking at the current tip, I don't think this is the correct fix, nor did I alter any of the logic in nativegen.py, so I'm not sure what's going on here.  It's possible that something is missing from the inheritance chain.

In any case, with this little hack, the tr-serrano builds (32-bit MacOS debug) and passes most of the regression tests.   I only changed six files, and diffing them against rev 5711, the subsequent changes seem reasonably contained and transparent, and I think it will be reasonble to audit these for unintended injections.  We still need to be careful about losing fixes that might have applied to the reverted code, had it been present, but the number of changsets affecting each of the files since 5711 is also reasonably small.   I think a complete reversion may be feasible after all.

Comment 13

[William Maddox]

Attachment: Failing tests after patch in attachment 554003

Regress/bug_654807 should be an expected fail, as it is testing for behavior that we intend to revert.

The JSON test failures deserve attention -- they are failing in a rather odd way.

Comment 14

[William Maddox]

One glitch that I forgot to mention: There were two blocks of code added to core/ArrayObject.cpp guarded by "#ifdef VMCFG_AOT", added in rev 5862. These may need some adjustments, as I've not attempted an AOT build.

Comment 15

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

\(In reply to William Maddox from comment #13)
> Created attachment 554005 [details]
> Failing tests after patch in attachment 554003 [details] [diff] [review]
> 
> Regress/bug_654807 should be an expected fail, as it is testing for behavior
> that we intend to revert.
> 
> The JSON test failures deserve attention -- they are failing in a rather odd
> way.

I think I ran into this exact problem when I was attempting a similar methodology either yesterday morning or the day before.  Its because you're missing an override of a virtual method in ScriptObject, toArray I think, that was introduced after the dense-array changes went in.

)I haven't looked at your patch yet, I just saw this part of your comment and decided to chime in immediately.)

Comment 16

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

Comment on attachment 554003 [details] [diff] [review]
Buildable, mostly working reversion patch

The patch reminds me of where I was 1.5 days ago or so.  :|

I'm F+'ing this not because I think it is the right path, but rather because I cannot convince myself that it is the wrong path.

(I've just spent a while on a different path.  Its probably a decent idea for Bill to finish up this approach, and I'll finish my latest one, which is to keep the dense-array code but super-impose the old length updating behavior.)

As alluded to above, for the JSON issue, you need to put back the virtual toArrayObject() method in ArrayObject.h

Comment 17

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

Attachment: patchA v4: buildable, "working" but inefficient, *isolated* reversion patch

Again, not asking for feedback on this one.

I'm posting it here because its interesting as a patch (it focuses in on a semi-minimal changeset that brings back the old dense-prefix-sparse-suffix representation without throwing away the infrastructure for the old one).

But if we do actually go all the way to reverting to dense-prefix-sparse-suffix representation, then something like what Bill posted is more likely to be the way to go, because this patch is ad-hoc and missing some of the efficiency short-cuts than Bill's approach inherently included.

Comment 18

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

Attachment: Extraction of m_length-mutating methods circa rev 5711

Extraction of ArrayObject m_length-mutating methods circa rev 5711

I've kept it in C++, but eliminated all of the components that are not relevant to m_length updates.

One item of potential interest: a method like unshift used to *sometimes* call setLength, and sometimes it directly modified the m_length.  If we want to be really careful, we'll need to preserve the logic of how that decision was made.  :(

Comment 19

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

(In reply to Felix S Klock II from comment #17)
> Created attachment 554115 [details] [diff] [review]
> patchA v4: buildable, "working" but inefficient, *isolated* reversion patch

I came up with tests that show that this patch (that is, the one I authored) is not semantics-preserving when compared against rev 5711.  The added tests were posted at Bug 677923, comment 15

Comment 20

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

(In reply to Felix S Klock II from comment #18)
> Created attachment 554135 [details]
> Extraction of m_length-mutating methods circa rev 5711

While this approach may work, its not as easy as I had hoped to layer atop the new dense-array rep, because the control flow logic here can depend on the old dense-prefix/sparse-suffix split, which in turn depends on the policy driving where the divide between dense and sparse falls, and that in turn depends (I think) on the history of the computation -- the necessary info is not generally derivable from the new dense-array representation, at least not in its current form.

Still, I'm going to give it a shot (seeing if I can get away with adding a single uint32_t member that tracks where the dense/sparse divide would have fallen if we were still using the old code.)

Comment 21

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

Attachment: patchB v1: superimpose old behavior atop new dense-array rep

Based on investigation outlined in attachment 554135 [details].  Superimposes old behavior atop new dense-array rep, by maintaining an artificial member representing the previous divide between dense/sparse spaces in the array, and then directly plugging in the old m_length update code (warts and all!  namely the bogus overflows that they had) based on the old case analysis from the old pre-dense-array code.

Comment 22

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

(In reply to Felix S Klock II from comment #21)
> Created attachment 554204 [details] [diff] [review]
> patchB v1: superimpose old behavior atop new dense-array rep

Forgot to mention: this is still work-in-progress.  In particular, while it passes the tests from arraydim.as v3 (attachment 553953 [details] [diff] [review]), it does not pass the new tests added in arraydim.as v4 (attachment 554115 [details] [diff] [review]).

Comment 23

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

Attachment: testW v1: Worrisome edge behavior, beyond length semantics

Worrisome behavior on edge cases, beyond the semantics of .length, *and* clearly dependent on subtleties in how the old code's policy decided on the dividing point between dense/sparse (which is something we can't easily emulate in the new code).

This is basically the breaking point that has led me to think that I should stop thinking we have any choice but to revert the dense-array code.

Here's the output I get from running it:

java -jar /Users/fklockii/Dev/FlashPFarce/tools/asc/asc.jar -import /Users/fklockii/Dev/Bugz/bugz679102/tamarin-redux/objdir-ged64/../generated/builtin.abc -import /Users/fklockii/Dev/Bugz/bugz679102/tamarin-redux/objdir-ged64/../generated/shell_toplevel.abc -AS3 /tmp/worrisome.as

worrisome.abc, 1356 bytes written
shell 1.4 debug-debugger build cyclone
features AVMSYSTEM_64BIT;AVMSYSTEM_UNALIGNED_INT_ACCESS;AVMSYSTEM_UNALIGNED_FP_ACCESS;AVMSYSTEM_LITTLE_ENDIAN;AVMSYSTEM_AMD64;AVMSYSTEM_MAC;AVMFEATURE_DEBUGGER;AVMFEATURE_ALLOCATION_SAMPLER;AVMFEATURE_JIT;AVMFEATURE_ABC_INTERP;AVMFEATURE_SELFTEST;AVMFEATURE_EVAL;AVMFEATURE_PROTECT_JITMEM;AVMFEATURE_SHARED_GCHEAP;AVMFEATURE_STATIC_FUNCTION_PTRS;AVMFEATURE_MEMORY_PROFILER;AVMFEATURE_CACHE_GQCN;AVMTWEAK_EXACT_TRACING;
ORIG a   [1]
ORIG sa1 [first,,,,,,,,,,sparse]
ORIG sa2 [,secnd,,,,,,,,,sparse]
ORIG da  [dense]
LARG a   []
LARG sa1 []
LARG sa2 []
LARG da  []
S8   a.length: 0 sa1.length: 0 sa2.length: 5 da.length: 0
PUSH a   []
PUSH sa1 []
PUSH sa2 [8,8,8,8,8]  <--------- WORRISOME BEHAVIOR
PUSH da  []
   a["0"]: 8
 sa1["0"]: 8
sa1["10"]: sparse
 sa2["0"]: 8
sa2["10"]: sparse
  da["0"]: 8

shell 2.0 debug-debugger build 6351:9a76f644c514
features AVMSYSTEM_64BIT;AVMSYSTEM_UNALIGNED_INT_ACCESS;AVMSYSTEM_UNALIGNED_FP_ACCESS;AVMSYSTEM_LITTLE_ENDIAN;AVMSYSTEM_AMD64;AVMSYSTEM_MAC;AVMFEATURE_DEBUGGER;AVMFEATURE_ALLOCATION_SAMPLER;AVMFEATURE_JIT;AVMFEATURE_ABC_INTERP;AVMFEATURE_SELFTEST;AVMFEATURE_EVAL;AVMFEATURE_PROTECT_JITMEM;AVMFEATURE_SHARED_GCHEAP;AVMFEATURE_MEMORY_PROFILER;AVMFEATURE_CACHE_GQCN;AVMFEATURE_SAFEPOINTS;AVMFEATURE_SWF12;AVMFEATURE_SWF13;AVMTWEAK_EXACT_TRACING;
ORIG a   [1]
ORIG sa1 [first,,,,,,,,,,sparse]
ORIG sa2 [,secnd,,,,,,,,,sparse]
ORIG da  [dense]
LARG a   [1]
LARG sa1 [first,,,,,,,,,,sparse]
LARG sa2 [,secnd,,,,,,,,,sparse]
LARG da  [dense]
S8   a.length: 6 sa1.length: 10 sa2.length: 10 da.length: 6
PUSH a   [1,8,8,8,8,8]  <--------- WORRISOME BEHAVIOR
PUSH sa1 [first,,,,,,,,,,sparse,8,8,8,8,8]  <--------- WORRISOME BEHAVIOR
PUSH sa2 [,secnd,,,,,,,,,sparse,8,8,8,8,8]  <--------- WORRISOME BEHAVIOR
PUSH da  [dense,8,8,8,8,8]  <--------- WORRISOME BEHAVIOR
   a["0"]: 1
 sa1["0"]: first
sa1["10"]: sparse
 sa2["0"]: undefined
sa2["10"]: sparse
  da["0"]: dense

Comment 24

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

Dammit; for future reference, here's the command I used to generate the above:

% ~/bin/asc -AS3 /tmp/worrisome.as && ~/Dev/Bugz/bugz679102/tr.r5711/objdir-ged64/shell/avmshell -Dversion ; ~/Dev/Bugz/bugz679102/tr.r5711/objdir-ged64/shell/avmshell /tmp/worrisome.abc  ; echo ; ./objdir-ged64/shell/avmshell -Dversion ;  ./objdir-ged64/shell/avmshell /tmp/worrisome.abc

Comment 25

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

(In reply to Felix S Klock II from comment #23)
> Created attachment 554229 [details]
> testW v1: Worrisome edge behavior, beyond length semantics
> 
> [...]
>
> Here's the output I get from running it:

At its heart, I think the differences in the two outputs stem from differences in how m_length is handled.  My main worry is that I am not sure we can reasonably emulate the old m_length maintenance in all cases.

Comment 26

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

Attachment: patchB v2: superimpose old behavior atop new dense-array rep

Comment 27

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

(In reply to Felix S Klock II from comment #26)
> Created attachment 554234 [details] [diff] [review]
> patchB v2: superimpose old behavior atop new dense-array rep

With this patch in place, the output from the worrisome.abc test (comment 23) is:

shell 2.0 debug-debugger build 6356:3b62fcf1f12b
features AVMSYSTEM_64BIT;AVMSYSTEM_UNALIGNED_INT_ACCESS;AVMSYSTEM_UNALIGNED_FP_ACCESS;AVMSYSTEM_LITTLE_ENDIAN;AVMSYSTEM_AMD64;AVMSYSTEM_MAC;AVMFEATURE_DEBUGGER;AVMFEATURE_ALLOCATION_SAMPLER;AVMFEATURE_JIT;AVMFEATURE_ABC_INTERP;AVMFEATURE_SELFTEST;AVMFEATURE_EVAL;AVMFEATURE_PROTECT_JITMEM;AVMFEATURE_SHARED_GCHEAP;AVMFEATURE_MEMORY_PROFILER;AVMFEATURE_CACHE_GQCN;AVMFEATURE_SAFEPOINTS;AVMFEATURE_SWF12;AVMFEATURE_SWF13;AVMTWEAK_EXACT_TRACING;
ORIG a   [1]
ORIG sa1 [first,,,,,,,,,,sparse]
ORIG sa2 [,secnd,,,,,,,,,sparse]
ORIG da  [dense]
LARG a   []
LARG sa1 []
LARG sa2 []
LARG da  []
S8   a.length: 0 sa1.length: 0 sa2.length: 0 da.length: 0
PUSH a   []
PUSH sa1 []
PUSH sa2 []
PUSH da  []
   a["0"]: 8
 sa1["0"]: 8
sa1["10"]: sparse
 sa2["0"]: 8
sa2["10"]: sparse
  da["0"]: 8

Comment 28

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

Attachment: patchB v2: superimpose old behavior atop new dense-array rep

(I uploaded an outdated patch file before, this should be the right one.)

Comment 29

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

Attachment: patchC v1: put back expected failure when we roll back

Put back expected failure for Bug 345961 when we roll back

(one step forward, two steps back, as they say...) :(

Comment 30

[William Maddox]

Attachment: Straightforward reversion patch

This is a straightforward reversion, intended to have minimal impact on unrelated code, and to be reasonable to review.  The The patch is against tamarin-redux-serrano.  It consists of a few adjustments to a previously-posted patch, and was constructed primarily by applying diffs, rather than
trying to manually untangle the changes.

For review, I recommend applying the patch and then doing 'hg diff -r 5711 <file>' where <file> is one of ArrayObject.[cpp,h], ArrayObject-inlines.h, and ArrayClass.[cpp,h].  It shouldn't be too difficult to match up the observed differences with changesets made following revision 5712.  If you do this with ClassClosure.cpp, you will see more changes than you want to read.  It will be easier to just note that the most recent changeset 6245 is the one that was reverted and to examine that patch.

The patch looks good on a Tamrin buildbot sandbox.  I corrected one issue with the VMCFG_AOT configuration detected in a Flash Player sandbox build, and have kicked off another, but the queue is pretty clogged up write now as I write this.  I am therefore only asking for feedback, but I think this patch is nearly reviewable, with AOT support being the main concern, and I'm pretty confident it's close, if not there.

Comment 31

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

Attachment: patchB v3: superimpose old behavior atop new dense-array rep

Fixed some bugs in the m_length maintenance patch.  (There are subtleties when calling the setLength method.)

Passes the acceptance tests (tested on a 64-bit DebugDebugger avmshell build).

Will post updated version of testW shortly.

Comment 32

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

Attachment: testW v2: Worrisome edge behavior, beyond length semantics

(the previous version was over-eagerly tagging output as worrisome;
this version focuses attention on the detail I'm concerned about.)

Again, compile it with -AS3 option to asc.jar.

Output from avmshell TR-serrano rev 5711:93a6f1d1128f (i.e., pre dense-array)
-----------------------------------------------------------------------------
ORIG a   [filllllllllllllllller]
ORIG sa1 [first,,,,,,,,,,sparse]
ORIG sa2 [,secnd,,,,,,,,,sparse]
ORIG da  [dense_filllllllllller]
LARG a   []
LARG sa1 []
LARG sa2 []
LARG da  []
S8   a.length: 0 sa1.length: 0 sa2.length: 5 da.length: 0
PUSH a   []
PUSH sa1 []
PUSH sa2 [8,8,8,8,8]  <--------- WORRISOME BEHAVIOR
PUSH da  []
   a["0"]: 8
 sa1["0"]: 8
sa1["10"]: sparse
 sa2["0"]: 8
sa2["10"]: sparse
  da["0"]: 8

Output from avmshell TR-serrano rev 6354:5e6ce1b8a224 (i.e., post dense-array)
------------------------------------------------------------------------------
ORIG a   [filllllllllllllllller]
ORIG sa1 [first,,,,,,,,,,sparse]
ORIG sa2 [,secnd,,,,,,,,,sparse]
ORIG da  [dense_filllllllllller]
LARG a   [filllllllllllllllller]
LARG sa1 [first,,,,,,,,,,sparse]
LARG sa2 [,secnd,,,,,,,,,sparse]
LARG da  [dense_filllllllllller]
S8   a.length: 6 sa1.length: 10 sa2.length: 10 da.length: 6
PUSH a   [filllllllllllllllller,8,8,8,8,8]
PUSH sa1 [first,,,,,,,,,,sparse,8,8,8,8,8]
PUSH sa2 [,secnd,,,,,,,,,sparse,8,8,8,8,8]
PUSH da  [dense_filllllllllller,8,8,8,8,8]
   a["0"]: filllllllllllllllller
 sa1["0"]: first
sa1["10"]: sparse
 sa2["0"]: undefined
sa2["10"]: sparse
  da["0"]: dense_filllllllllller

Output with patchB-v3-superimpose-old-length i.e. attachment 554370 [details] [diff] [review]
-------------------------------------------------------------------
ORIG a   [filllllllllllllllller]
ORIG sa1 [first,,,,,,,,,,sparse]
ORIG sa2 [,secnd,,,,,,,,,sparse]
ORIG da  [dense_filllllllllller]
LARG a   []
LARG sa1 []
LARG sa2 []
LARG da  []
S8   a.length: 0 sa1.length: 0 sa2.length: 0 da.length: 0
PUSH a   []
PUSH sa1 []
PUSH sa2 []
PUSH da  []
   a["0"]: 8
 sa1["0"]: 8
sa1["10"]: sparse
 sa2["0"]: 8
sa2["10"]: sparse
  da["0"]: 8

Comment 33

[Trevor Baker]

Am told there is aot code interspersed in the patch in comment #30, so asking Alok Manchanda for feedback.

Comment 34

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

Comment on attachment 554310 [details] [diff] [review]
Straightforward reversion patch

Looks good.

I've asked Trevor to get some performance comparison results atop this patch, so that we have an idea about how much things will regress (if at all) by putting this in.

Comment 35

[Brent Baker]

Gathering data for v8.5 and scimark, which matches up with a noted performance drop that was tracked back to the dense array work (WE:2884308, BZ bug# 654807)

Comment 36

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

(In reply to Brent Baker from comment #35)
> Gathering data for v8.5 and scimark, which matches up with a noted
> performance drop that was tracked back to the dense array work (WE:2884308,
> BZ bug# 654807)

dense-arrays were *supposed* to be a win for some performance cases.  It would be good to cover the cases where it was supposed to win as well as the cases where we have learned that it loses (or at least, loses with the hack that steven put in for non-dynamic subclasses)

In particular, look over the results Steven posted at Bug 610063

My understanding there were cases there with big wins; make sure to include those as well as the known losses.

Comment 37

[Brent Baker]

Attachment: Perf comparison between tr-ser-sec 6499:0e891e91254f with and without patch

Overview of perf change with the attachment id# 554310. This is trying to mirror original data reported here:
https://bugzilla.mozilla.org/show_bug.cgi?id=610063#c30

This is a highlight of the main changes with noise removed, full log is attached. Most of the big wins reported in 610063#30 have been lost with this change, which is expected.

brbaker-MacBookPro:performance brbaker$ ./runtests.py -i 5 
Tamarin tests started: 2011-08-19 10:05:53.624673
Executing 460 test(s)
avm: /Users/brbaker/hg/tamarin-redux-serrano-security/objdir32/shell/avmshell  version: 6499:0e891e91254f
avm2: /Users/brbaker/hg/tamarin-redux-serrano-security/objdir32_patch/shell/avmshell  version: 6499:0e891e91254f
iterations: 5

                                        avm            avm2
test                           best     avg    best     avg   %dBst   %dAvg
Metric: iterations/second 
Dir: asmicro/
  alloc-1                      38.2    36.6    36.7    35.0    -3.9    -4.4   
  alloc-11                     17.8    17.7    17.4    17.2    -2.7    -3.1 - 
  alloc-12                     17.7    17.4     9.0     8.6   -49.1   -50.6 --
  alloc-2                      23.1    22.9    23.7    23.5     2.4     2.4 + 
  alloc-3                      15.7    15.4    15.6    15.3    -1.2    -0.5 - 
  alloc-5                      33.8    32.8    31.6    31.2    -6.7    -4.9 --
  arguments-3                  21.9    21.8    22.4    22.2     2.1     1.9 + 
  array-1                    1542.5  1506.5  1637.4  1593.2     6.2     5.8 + 
  array-pop-1                 445.1   416.8   550.9   507.4    23.8    21.7 + 
  array-push-1                361.3   359.4   386.6   384.8     7.0     7.1 ++
  array-slice-1                20.6    20.4    18.4    18.1   -10.7   -11.3 --
  array-sort-2                  2.7     2.7     2.7     2.7     1.0     0.7 + 
  array-unshift-1             205.4   203.4   194.4   193.2    -5.3    -5.0 --
  for-in-2                    404.2   397.8   162.8   161.4   -59.7   -59.4 --
  globalvar-write-1          5672.3  5618.6  5873.1  5730.7     3.5     2.0 + 
  number-toString-1             7.0     7.0     6.9     6.8    -1.4    -1.9 - 
  oop-1                         4.0     3.9     3.9     3.8    -1.4    -2.6 - 
  regex-exec-1                 65.3    63.6    70.6    69.0     8.0     8.5 + 
  regex-exec-2                 79.8    78.7    83.7    83.1     5.0     5.5 ++
  regex-exec-4                277.4   268.6   334.7   288.6    20.6     7.4 ++
  restarg-2                   500.0   498.7   514.0   512.8     2.8     2.8 + 
  string-indexOf-1            206.2   204.2   221.1   210.9     7.2     3.3 ++
  try-3                        56.3    56.2    56.9    56.7     1.0     1.0 + 
Metric: time 
Dir: jsbench/
  HeapSort                   2812.0  2821.0  2896.0  2912.0    -3.0    -3.2 - 
  alloc-12                     11.9    11.6     6.3     6.1   -47.6   -47.7 --
  alloc-9                      11.6    11.4    11.9    11.7     2.7     2.7 + 
  arguments-3                  19.1    19.0    19.5    19.4     2.2     1.9 + 
  array-2                     261.7   257.7   276.4   267.8     5.6     3.9 + 
  array-push-1                 54.5    53.6    52.6    52.2    -3.5    -2.7 - 
  array-slice-1                16.8    15.8    14.8    14.1   -12.2   -10.8 --
  array-sort-3                 21.3    17.9    17.9    17.2   -15.8    -4.0 - 
  array-unshift-1              24.9    24.6    25.5    25.2     2.2     2.4 + 
  for-in-2                    315.1   313.7   142.7   142.0   -54.7   -54.7 --
  number-toString-1             6.6     5.6     5.4     5.4   -17.4    -3.4 --
  number-toString-2            54.3    51.9    60.5    54.1    11.3     4.2 + 
  oop-1                         3.9     3.9     4.0     4.0     1.8     1.1 + 
  string-casechange-2          16.0    15.5    15.8    15.6    -1.4     0.5 - 
  string-indexOf-2             56.9    53.9    64.7    54.8    13.8     1.8 + 
  string-lastIndexOf-1         90.1    89.6    88.0    87.5    -2.3    -2.3 - 
  string-split-1               10.0     9.9     9.8     9.8    -1.2    -1.0 - 
  string-split-2                9.7     9.5     9.5     9.5    -1.4    -0.5 - 
  try-1                       197.8   195.9   194.2   193.5    -1.8    -1.2 - 
  try-2                        14.6    13.2    12.5    12.3   -14.7    -6.6 --
Dir: language/string/
  split                       336.0   342.0   344.0   345.4    -2.4    -1.0 - 
  static_ascii_array_100      183.0   186.8   866.0   882.2  -373.2  -372.3 --
  static_ascii_array_50       168.0   171.0   853.0   860.2  -407.7  -403.0 --
  static_latin1_array_100     416.0   428.6  2040.0  2097.6  -390.4  -389.4 --
  static_latin1_array_50      218.0   222.6  1014.0  1049.0  -365.1  -371.2 --
Dir: language/string/typed/
  replace                     428.0   438.6   457.0   464.4    -6.8    -5.9 --
  search                       35.0    35.4    32.0    33.6     8.6     5.1 + 
  split                       404.0   410.2   417.0   425.4    -3.2    -3.7 - 
Dir: misc/
  boidshack                   362.0   367.8   422.0   423.0   -16.6   -15.0 --
  gameoflife                 1506.0  1619.0  2165.0  2398.6   -43.8   -48.2 --
Dir: scimark/
  FFT                        2009.0  2291.6  1956.0  2205.8     2.6     3.7   
  LU                         2510.0  2588.0  2530.0  2675.8    -0.8    -3.4   
  MonteCarlo                 2343.0  2656.4  2311.0  2605.6     1.4     1.9   
  SOR                        2271.0  2283.4  2178.0  2265.8     4.1     0.8   
  SparseCompRow               109.0   112.6    89.0    98.2    18.3    12.8 + 
Dir: sunspider/
  access-fannkuch              70.0    72.6    78.0    84.0   -11.4   -15.7 - 
  access-nsieve                24.0    26.0    46.0    53.4   -91.7  -105.4 --
  bitops-bitwise-and          306.0   325.4   290.0   300.4     5.2     7.7 + 
  s3d-morph                    32.0    32.0    31.0    31.8     3.1     0.6 + 
Dir: sunspider/as3/
  access-fannkuch              38.0    38.4    41.0    41.6    -7.9    -8.3 --
  access-nsieve                12.0    12.8    30.0    31.0  -150.0  -142.2 --
  bitops-bits-in-byte           8.0     8.0     7.0     7.6    12.5     5.0 + 
Dir: sunspider-0.9.1/js/
  access-fannkuch              60.0    61.4    66.0    66.4   -10.0    -8.1 --
  access-nsieve                20.0    20.6    38.0    38.6   -90.0   -87.4 --
  crypto-sha1                  15.0    15.0    14.0    14.8     6.7     1.3 ++
  math-cordic                  42.0    43.2    44.0    44.0    -4.8    -1.9 - 
Dir: sunspider-0.9.1/typed/
  access-nbody                  3.0     3.8     4.0     4.0   -33.3    -5.3 - 
  access-nsieve                11.0    13.4    34.0    37.4  -209.1  -179.1 --
  bitops-bitwise-and            2.0     2.0     1.0     1.2    50.0    40.0 + 
  bitops-nsieve-bits           21.0    21.4    20.0    20.8     4.8     2.8 + 
  crypto-md5                    3.0     3.0     2.0     2.8    33.3     6.7 ++
  regexp-dna                  711.0   718.4   686.0   696.0     3.5     3.1 + 
  string-fasta                 26.0    26.4    24.0    25.0     7.7     5.3 + 
Metric: v8 custom v8 normalized metric (hardcoded in the test)
Dir: v8/typed/
  earley-boyer                846.0   821.4  1054.0   856.4    24.6     4.3 + 
Dir: v8.5/optimized/
  raytrace                   6642.0  6528.2  7182.0  6988.0     8.1     7.0 ++
Dir: v8.5/typed/
  crypto                     2642.0  2438.6  3144.0  2574.6    19.0     5.6 +

Comment 38

[Brent Baker]

Attachment: Matching perf data to original data posted by Steven

Report on the same testcases as was originally noted in https://bugzilla.mozilla.org/show_bug.cgi?id=610063#c30

Comment 39

[William Maddox]

I would expect to see substantial regressions, based on Steven's benchmarks justifying the original dense-array work.  The most relevant comparison would be against our last shipped release, at least insofar as checking for actual regressions goes.  The potential performance gain we leave on the table for Serrano would be part of the cost of doing the reversion, but not a regression in the field.

Comment 40

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

After discussion with management, we've decided that rolling back the dense array changes is likely to do more harm than good.