604657 - performance injection in sunspider javascript version of tests

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Dan Schaffer]

in salt regression testing, detected a performance degradation in these 2 tests:
sunspider-0.9.1/js/math-partial-sums.as
sunspider-0.9.1/js/bitwise-and.as

** math-spectral-norm.as and string-validate-input.as also showed ~10% degradations all other js/ tests were within normal range.  

tracked down to tr-sec 5438


$ ./runtests.py --avm=/Users/dschaffe/hg/tamarin-redux-security/objdir-release-5437/shell/avmshell  --avm2=/Users/dschaffe/hg/tamarin-redux-security/objdir-release-5438/shell/avmshell --iterations=5 sunspider-0.9.1/js/bitops-bitwise-and.as  sunspider-0.9.1/js/math-partial-sums.as 
Tamarin tests started: 2010-10-15 11:13:16.873215
Executing 2 test(s)
avm: /Users/dschaffe/hg/tamarin-redux-security/objdir-release-5437/shell/avmshell  version: cyclone
avm2: /Users/dschaffe/hg/tamarin-redux-security/objdir-release-5438/shell/avmshell  version: cyclone
iterations: 5

                                      avm          avm2
test                          best    avg   best    avg  %dBst  %dAvg
Metric: time 
Dir: sunspider-0.9.1/js/
  bitops-bitwise-and           236  242.4    387    397  -64.0  -63.8 --
  math-partial-sums            210    220    335  340.6  -59.5  -54.8 --


rev 5438 in tr-sec is:
changeset:   5438:73e906124c45
user:        Steven Johnson <[e-mail]>
date:        Fri Sep 03 10:54:01 2010 -0700
summary:     Bug 567284 - Domain lookup rules are inconsistent and buggy (r=edwsmith,rreitmai)

Comment 1

[Dan Schaffer]

I reproduced the problem in the salt release in p4 //depot/main/FlashRuntime/Milestones/Argo_Athena_GMC/code/third_party/avmplus/...

baseline changelist 710658 : runs fast
next avmplus changelist: 714324 : exhibits the slowdown (also I had to apply the changes from 724958, updates to DomainClass,cpp, avmshell.h, ShellCore.cpp, and ShellCore.h to build the shell)

all revisions after 714324 including the salt exhibit the slowdown

Comment 2

[Dan Schaffer]

I wanted to point out this reproduces only on the javascript raw versions of sunspider and there is not a performance degradation on the typed versions of the same sunspider tests.

Comment 3

[Steven Johnson]

OK, this is an interesting one; these benchmarks repeatedly access a global JS var:

            function runBitopsBitwiseAnd() {
              // bitwiseAndValue is never explicitly declared
              bitwiseAndValue = 4294967296;
              for (var i = 0; i < 600000; i++) {
                bitwiseAndValue = bitwiseAndValue & i;
              }
              return bitwiseAndValue;
            }

So every time we access it, we call findglobalproperty(), which goes thru the look-up-and-down-the-cached-defs lookup in DomainMgr. In theory, we cache the name found the first time we find it, so subsequent lookups are cheap... but in this case, the name isn't found, and our scheme *doesn't* cache "not-found-anywhere", so *every* subsequent call has to repeat the up-and-down lookup scheme to determine that the name is still not found. Eww.

The obvious thing that springs to mind is to smarten the logic so that a not-found name is actually cached somehow...

Comment 4

[Edwin Smith]

presumably the negative cache would be cleared aggressively, possibly by hooking into AvmCore::invalidateLookupCache().

Comment 5

[Steven Johnson]

(In reply to comment #4)
> presumably the negative cache would be cleared aggressively, possibly by
> hooking into AvmCore::invalidateLookupCache().

That's a good question, and one I'm not sure I know the answer to. It could be argued that freeze-on-first-use should apply to not-found as well as found, in which case we'd want to not clear it.

I'm playing with a prototype for this but any such change is far too late to consider for Salt.

Comment 6

[Avik Chaudhuri]

I don't think soundness is affected by having a negative cache. Some functionality may be lost, e.g., if a parent domain loads a definition between two queries from a child; however, such use-cases may be uncommon enough to ignore or weigh against how common use-cases such as the benchmark above are.

Another thing to consider is whether clearing the negative cache can affect soundness. Again, I don't think it does: a definition that is not found now can either be not found later (by negative caching or otherwise), or found later (by clearing the cache) and remain found in the future.

Comment 7

[Tommy Reilly]

I think Edwin was alluding to the fact that a program could look up lots of undeclared names and we dont want the cache to blow up.   Like maybe a fixed size LRU cache would be appropriate.

Comment 8

[Dan Smith]

Why is this bug classified as a security concern?  It is related to bug 567284 but if the soundness of the lookup is not questioned, can this be declassified?