416924 - TT: Existing "hook" override mechanism is inefficient

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Steven Johnson]

Attachment: Patch

the mechanism for overriding get/set/has/del prop, as well as call/construct/next etc, is quite inefficient, especially for Array. We can rewrite it and get substantially smaller traces.

Comment 1

[Steven Johnson]

Big grab-bag of speed improvements, mostly centered around re-doing the "hook" mechanism for overriding get/set/has/del property, and changing the internals of Array.

In turns out that the overhead from calling to Forth back into AS3 for getProperty (etc) had nontrivial overhead, especially for Array. Added Forth-level special-casing (based on TraitsEnv builtintype field) to avoid the necessity for this in most cases. This also allowed us to short-circuit numeric cases for Array more simply. The upshot is that Array get/set is much much faster now, but it can probably get faster still (there is more overhead that can be streamlined or removed, that I hope to do in the future).

In addition, revamped the internals of Array to assume dense is more likely than sparse. (This needs more work; the heuristics for dense vs spare are a little arbitrary right now.) And, we now store dense arrays as lists of Box rather than Atom (this is a huge win for array of doubles as we don't atomize them). As a results, AtomArray vanished entirely (we now use an augmented version of List<> for the dense portion, and E4XNode now uses appropriately-typed List<> for its own purposes).

Similar optimization went in for the call and construct hooks, and nextname/nextvalue/nextindex.

toString/valueOf/defaultValue aren't done, but are candidates for future work.

Also revised the native method interface so that methods returning untyped values now return Box::RetType rather than Atom. This is again a nice win for doubles but requires a new build of GlobalOptimizer (and hence a new asc.jar).

Also removed the InternedStringWithIntAtom construct, as it was no longer a performance win (it was useful in TC as runtime values had to be stored inside of Multinames, but our code path no longer necessitates that)... this saved us a bit of time and space, and I recycled the freed bit to indicate if a string was all single-byte UTF8, which gave dramatic speedups in a few benches that relied on chatAt() and friends. (More work needs to be done on String performance though.)

also added a trick for methods with no type information for any args: detect this and skip the coerce_actual_args loop entirely, which can be time consuming.

fixed a subtle Forth bug in that functions with ignore_rest set didn't have the extra args popped properly... which could cause weird tracing behavior in a few cases. this was really hard to track down...

other notes:
-- struct Box got moved from Interpreter.h to its own header file.
-- List<>.removeAt had a bug that was not previously reported. Fixed.
-- didn't test with spam, only with mmgc, so changes might need tweaking there.
-- did some smartening of various verbose-output permutations and of debug-mode boxtype invalidation.
-- changed getbinding2, globalobj2, isbadinit2 to be PURE (not CSE)... they should have been before, just was a mistake
-- net size gain of about 300 bytes on ARM build, but well worth it for perf gains realized.

Comment 2

[Steven Johnson]

Bench numbers of before-and-after for a Release build with -Dnoloops:

                                TT-OLD  TT-NEW
prime4                          197     192
boidsHeadless                   3588    3144
boidshackHeadless               1664    562
DrawGridHeadless                2405    594
access-binary-trees             274     246
access-fannkuch                 632     222
access-nbody                    314     298
access-nsieve                   1059    141
bitops-3bit-bits-in-byte        40      26
bitops-bits-in-byte             98      90
bitops-bitwise-and              412     387
bitops-nsieve-bits              196     89
crypto-md5                      4672    2920
crypto-sha1                     1596    936
math-cordic                     113     90
math-partial-sums               360     339
math-spectral-norm              115     75
s3d-cube                        1545    461
s3d-morph                       159     123
s3d-raytrace                    1899    610
string-fasta                    497     279

Comment 3

[Edwin Smith]

Comment on attachment 302730 [details] [diff] [review]
Patch

* in Array.as "var uv:uint = uint(V);" uint(V) is unnecessary in AS3, assignment to :uint var is enough.
* doing uint(V) will call V.valueOf or V.toString which has side effects, is this legal? or is V always primitive?
* I'm leery of [ctor_is_stub].  G.O. shouldn't be changing the signature, and changing needs_rest to ignore_rest is meaningful only inside the ctor function.  clients (asc?) must treat the two flags the same way.  Erik just pushed an asc fix.
* from what I can tell, a new empty array, e.g. [], is still not considered dense (isSimpleDense() returns false).  the fast path in push() (maybe others too) needs [] to be dense.  I'm not sure where the dense work is, progress wise, it's okay if it's out of scope for this patch.

Comment 4

[Steven Johnson]

Attachment: Patch

-- I removed all the redundant uint() casts from Array.as (most of these have been here a long tme)
-- after offline discussions with Edwin and Erik we determined that the issue that [ctor_is_stub] was addressing was really an underlying bug in the asc compiler itself; Erik has pushed a fix to asc (a new asc.jar is still required for this patch).
-- yeah, dense-array work is definitely NOT complete with this patch. I plan on revisiting it in a future patch. this has just enough done to allow for the revised array api I needed.

Comment 5

[Steven Johnson]

pushed as changeset:   303:d7deac103a9c

Note, a new build of asc.jar is required to build with these changes.

Comment 6

[Steven Johnson]

Attachment: Patch

A few fixes to my patch of yesterday:

core/AvmCore.cpp: remove bogus asserts (null is legal for error arguments)

core/Box.h: initializer syntax is invalid; mac and windows don't complain, but linux gcc does.

core/StringObject.cpp, .h: my earlier SINGLEBYTE fixes were incomplete (we were still sometimes processing single-byte strings the hard way).

platform/mac/shell/shell.xcodeproj/project.pbxproj: the "reference-only" references to MMgc and friends were no longer correct (needed to be updated for space/spam)

platform/win32/avmplus_9.sln: minor fixes to VS2008 build settings.
platform/win32/shell_9.vcproj: minor fixes to VS2008 build settings.
space/space_9.vcproj: minor fixes to VS2008 build settings.
space/spam/build/spam_9.vcproj: minor fixes to VS2008 build settings.

Comment 7

[Edwin Smith]

Comment on attachment 303092 [details] [diff] [review]
Patch

looks fine either way but, did you intened to change the mac project files?

Comment 8

[Steven Johnson]

Yes, see my comment above... all the avmplus and mmgc files were included as "reference" (not build) in the shell project, to make searching work easier (since XCode is kinda stupid and doesn't search subprojects).

Comment 9

[Steven Johnson]

pushed as changeset:   304:0dbe7a126eb8