440242 - SymTable can easily be faster

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Steven Johnson]

The new-string changes recently made in TT make it pretty easy to go back to have SymTable using interned-strings for the name portion of a key, meaning the compares are faster. See attached patch for details.

Comment 1

[Steven Johnson]

Attachment: Patch

Enclosed patch goes back to a TC-style lookup table for symbols... this gives a modest speedup on most of our simple benchmarks, but for tests with complex class hierarchies (eg Flex), it can improve startup time by 33% or more.

ifdef'ed for now until we're happy it's a good thing overall (ifdefs to be removed soon if so).

Also includes an increase in the TraitsData cache size from 64 to 128 (the speed improvement above takes this into account)

Comment 2

[Edwin Smith]

What is the memory impact of this?  SymTab's pointing into ABC data was for memory savings, not speed.  Doing this only for names, but not namespaces, was a 1/2-way done job... i would agree with an argument we should use abc data for both, or objects (strings+ns's) for both.  in big apps, symtab data is a big % of memory, iirc.

Comment 3

[Steven Johnson]

Michael's new string code went back to keeping a table of String objects (lazily allocated) for pool strings, but the strings point into the abc data, so overhead is relatively small (string objects + table space). And, I'm guessing that with a little cleverness we can remove the now-nearly-unused cpool_string_starts table. 

So, although I haven't measured the memory impact, I can believe that it's fairly small, for a sizable performance gain.

Comment 4

[Edwin Smith]

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

is there any overlap with Bug 424891?

it sounds like youre saying weve already taken the memory hit, but this makes it no worse.  

In any case, interned comparisons do RULE, lets go with it and not let down our guard on memory usage.

Comment 5

[Steven Johnson]

changeset:   418:6984d3bf9e71