617682 - AOT parsemultiname cache

Status: VERIFIED INVALID

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Alex Macdonald]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_5; en-us) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4
Build Identifier: 

To save a lot of redundant work during script execution we cache the results of "parseMultiname". The AOT compiler generates an array at compile time with one entry per multiname in the abc, The first time an AbcEnv is created for a given ABC we parse all the multinames. The patch also includes a couple of minor bug fixes.

Reproducible: Always

Comment 1

[Alex Macdonald]

Attachment: Aot parsemultiname caching

Comment 2

[Steven Johnson]

Comment on attachment 496215 [details] [diff] [review]
Aot parsemultiname caching

No obvious flaw, but I need to see AOTLazyEvalGuard to give a useful review, I suspect... where is it?

Comment 3

[Steven Johnson]

Comment on attachment 496215 [details] [diff] [review]
Aot parsemultiname caching

No obvious flaw; adding Lars to double-check for possible WB issues with initing Multinames in this way.

Comment 4

[Lars T Hansen]

Without a definition for AOTInfo it's not possible to tell.  I'll hold off on the review until that information is available.

Could you use the precomputedMultinames array instead, or somehow integrate the ideas?  Krzysztof just posted a patch which I R+'d for making the precomputed multinames always available.

Comment 5

[Lars T Hansen]

Comment on attachment 496215 [details] [diff] [review]
Aot parsemultiname caching

Impossible to tell if it's correct without access to the AOTInfo structure.  And again, could the precomputedMultinames not serve this purpos?

Comment 6

[Alex Macdonald]

sorry for the late response, I've started prepping the patch to open source the entirety of the AOT changes so you can see the AOTInfo structure in AOTCompiler.h in bug 620678.

We could potentially use the precomputed multiname support, although it would be somewhat slower (getting a static multiname is just a single load instruction for AOT'ed apps now).

Comment 7

[Trevor Baker]

bulk verifying resolved !fixed issues