424891 - TT: add a simple mnpos2ctname cache

Status: VERIFIED WONTFIX

(Tamarin Graveyard :: Tracing Virtual Machine, enhancement)

Description

[Graydon Hoare :graydon]

Attachment: patch implementing ct multiname cache

This patch wins between 5-15% on many of the sunspider benchmarks; it simply adds a bounded, linear move-to-front cache on the front of the mnpos2ctname function, which dominates a very hot subtree of the call graph profiles (> 99.9% repetitive calls).

It's a crude technique -- static variable with static GC root -- but I'd recommend adopting something *like* this. Review / suggestions about a more appropriate approach would be appreciated. For example, would it be better to stick the cache in the PoolObject?

(I notice that the TC PoolObject is quite different from the TT one; is there a plan to reconcile the code bases at some point?)

Comment 1

[Graydon Hoare :graydon]

Attachment: update to ct multiname cache patch

Minor update to same code; puts the GC root put on the interpreter stack rather than in a static. Still uses a static for the cache itself.

Comment 2

[Edwin Smith]

Do you know if the compile-time multinames being created are mostly being used to access dynamic properties?  There is already a cache for the case when you're looking up a binding (fixture) with a compile time name.  in that case we cache the Binding info with the name and the multiname is just an intermediate value only needed on a cache miss.

but for the case of looking up a dynamic property with a ct name, i could see the generated multiname living longer.  in that case, this cache makes sense.  another possibility would be to add to the existing cache -- an entry holding a binding that means "do a dynamic lookup using this multiname" 

Comment 3

[Steven Johnson]

Hey Graydon, I haven't poked into this yet but this smells like there may be an underlying bug here -- I wouldn't have thought that this would have been such a huge win.

On the other hand... sunspider is all ES3, so it's going to be all dynamic lookups anyway... so maybe this is just an optimization case that I missed in my first cut at caching stuff in the interpreter.

as far as details of the cache implementation, looks workable, but I wonder if we could get by with a fixed-size cache (a la QCache, which we use for TraitsEnvData) and avoid the overhead of allocating GC memory as we go along.

Comment 4

[Steven Johnson]

IMHO mn_cache should be a property of the Interpreter, not a global -- any reason it's not?

Otherwise, looks good

Comment 5

[Steven Johnson]

Hey, I tried reapplying this to current tip but find that it now is actually a performance drag rather than gain -- I think because the big savings from it was being able to bypass the call to getInternedPoolString(), which is now vastly cheaper due to the recent String rewrite. 

Comment 6

[Graydon Hoare :graydon]

Great. I was hoping this would turn out to be the case. Shall I close this, then? Ad-hoc caches are sort of warts, much preferable to just have an efficient subsystem.

Comment 7

[Steven Johnson]

Yeah, I think so; it might be worth revisiting someday but I think it's no longer the low-hanging fruit.