432487 - IL should be shared between multiple MethodEnv's of the same Function

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect, P1)

Description

[Edwin Smith]

Each MethodEnv captures a scope chain for an underlying function.  IL is currently (wrongly) associated with MethodEnv rather than the underlying code, so we end up duplicating IL for the function.

This is unbounded duplication.  Tracing magnifies the problem because if the functions call a hot subroutine, the many closures look like many call sites, causing much inlining, or if the subroutine has a loop, high fan-out on the path exiting the loop back to the caller.

ESC on TT demonstrates this problem with Hashtable.read() calling eqfn() to compare strings

Comment 1

[Steven Johnson]

What if we could have multiple instances of the same MethodEnv-with-different-scope just share pointers to the same IL? If they all refer to the same underlying code then it should be identical code (and if not then they need different IL chunks anyway)

I guess the issue is how to do that sharing; if we are always verified at least once before MethodEnv::cloneWithNewScope is called then it should be easy-peasy... but I don't think that is the case. 

Comment 2

[Steven Johnson]

Stealing this, I see a simple way to deal with it

Comment 3

[Edwin Smith]

Attachment: fixes this and other misc stuff

This code is stale, but addresses this bug.  highlights:

* lots of int->int32_t noise
* optimize emit_LIT to use DUP or OVER when possible
* specializations of get/set/findprop
* got rid of the "gmi" entry point, everything is in ckargs
* ops that take a nameid take a nameoff instead (offset into abc), one less thing to do at runtime
* get rid of everCalled()
* added OP_getouterscope

Comment 4

[Steven Johnson]

Attachment: Updated Patch

This encompasses part of your patch, but not all. 

Included:
* The basic IL-sharing fix, improved a bit to remove two more pointers from MethodEnv (storing 'em in AbcEnv is better since they won't ever be replicated, IMHO)
* optimize emit_LIT to use DUP or OVER when possible
* got rid of the "gmi" entry point, everything is in ckargs
* get rid of everCalled()

Not included:
* lots of int->int32_t noise
* specializations of get/set/findprop
* ops that take a nameid take a nameoff instead (offset into abc), one less
thing to do at runtime
* added OP_getouterscope

This gives a modest but measurable speed improvement on sunspider tests, probably due to increased efficiency of closures with this code.

Comment 5

[Steven Johnson]

Attachment: PendingConst improvement

Here's another patch (additive to previous one) to adapt the pending-const-speedup as well. This uses a new-and-improved technique different from the one in Edwin's initial patch.

Comment 6

[Steven Johnson]

pushed as changeset:   442:0e2c25c2537a