438330 - Interface override dispatch doesn't work

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Tommy Reilly]

See attached test case.

Comment 1

[Tommy Reilly]

Attachment: fixes attached test case

Would another way to tackle this to be able to change the look at verify time?

Comment 2

[Tommy Reilly]

More details:

callsite:

      46    findpropstrict	mx.core::IUIComponent
      48    getlocal1     	
      49    callproperty  	mx.core::IUIComponent (1)
      52    callpropvoid  	mx.core:IUIComponent::initialize (0)

public UIComponent::initialize  <- this one is called!!! initialize is also an interface method
 override public Container::initialize
    (LayoutContainer no override)
        override public Application::initialize
            override public HelloWorldFlex::initialize <- this one should be called!

Comment 3

[Steven Johnson]

Need to add a new sanity test for this to acceptance tests

Comment 4

[Tommy Reilly]

pushed 

Comment 5

[Tommy Reilly]

We want to revisit this fix.  Copying the interface names into the concrete class bindings fixes the problems but requires a deep inheritance tree walk everytime we build a TraitsData and has obvious space requirements.   Can we expose public bindings that are implementing interface names somehow without this?   Can the interface names be boiled away somehow so that at runtime we only have one set of names?     To tie the problem concretely to the example above, how do we get the name "mx.core::IUIComponent::initialize" to map to "HelloWorldFlex::initialize".

Comment 6

[Steven Johnson]

Yep. There are outright bugs in the current code in that we walk the deep hierarchy redundantly many times; we also don't prune redundant interfaces (which can occur frequently in Flex apps). I've fixed these in my local tree but performance is still not where it needs to be and the lookup tables are still getting pretty large. (The way we have to propagate protected namespaces has a somewhat similar issue.) I'm pondering improvements to this, will update bug as thoughts occur to me.

Comment 7

[Steven Johnson]

More info: currently, our TraitsData cache is getting a better than 98% hit rate for a cache size of 128 when loading a Flex app; unfortunately this is apparently too low given the time it takes to regenerate a typical TraitsData with the above namespace propagation required. (There are lots of classes that end up having TraitsData regenerated a dozen or two times during startup.) 

I'd prefer to avoid increasing the cache size, as the TraitsData cache is a nontrivial source of memory savings in TT, and 128 is already larger than I had hoped would be practical.

Is it reasonably possible to smarten our cache to substantially improve on a 98% hit rate, I wonder? (My knowledge of caching schemes is somewhat limited.)

Comment 8

[Tommy Reilly]

Theory: the only way to invoke an interface method through an interface name is when the interface type is used.  The two ways I could come up with are (C == class, I == interface, inst == instance):

I(inst).foo();
inst.I::foo();

In both cases the Verifier can identify I as an interface and convert both of these to:

inst.foo();

With the verifier making this reduction the hack I added is no longer necessary.  See anything wrong with this theory?  

Comment 9

[Steven Johnson]

Attachment: Patch

This patch includes an improvement for this situation, along with other misc speedups. We probably need to re-think this more thoroughly but the enclosed fix is a huge improvement over the status quo.

-- fixed horrible bugs in TraitsEnv  dealing with propagating protected namespaces and public->interface namespace mappings; in both cases we were recursing over the entire inheritance hierarchy many times, resulting in n^2 behavior -- oops. This improves perf dramatically on class-heavy suites (eg Flex) but more work is needed as the current design simply requires too much replication of data and too much work at cache-regen time; however, the speed improvement of this fix is too good to postpone.
-- added a bitfield to TraitsEnv to indicate which nativeHooks are implemented; this saves a SymTable lookup and is a major speed improvement in some cases (eg anything that used == a lot)
-- TraitsEnv::containsInterface now checks its direct inheritance tree before resorting to getting its TraitsData, as this will often satisfy the request
-- marked all the private native methods in our builtin objects as final as well, to improve runtime optimization opportunities
-- there were a couple of protected methods in Object.as that were only used by Object or Dictionary.as; since they were just thin wrappers around forth code I replicated them as necessary into Dictionary and made them all private. (Otherwise we had to propagate these into every SymTable in existence just in case a subclass wanted to call them in its own namespace...)
-- rewrote setDelegate and getDelegate to use the existing forth prims rather than native C code
-- rearranged some code in SymTable to ensure that string+ns comparisons were done inline (function calls were creeping in despite inline directives). Also added a _get1() case for the very very common case of looking up a single name+ns pair.
-- tweaked the pre_interp code in VMInterp.cpp to minimize the speed penalty for Release_Debugger builds (we always called pre_interp even if verbose wasn't on)

Comment 10

[Steven Johnson]

> In both cases the Verifier can identify I as an interface and convert both of
> these to:
> 
> inst.foo();
> 
> With the verifier making this reduction the hack I added is no longer
> necessary.  See anything wrong with this theory?  

No, but is it possible for an ABC file to define two incompatible definitions of the method in the two namespaces? Our existing code only promotes the public binding into the interface binding if it's a compatible kind and if it's different from the existing one, but it's not clear to me if this implies that the implementation will *always* be a public one, or if it would also be legal for it to be one specified (somehow) in the explicit interface namespace... eg,

    interface bar { public function barfunc(); }

    // the usual case
    class foo implements bar
    {
       public function barfunc(); 
    }

    class foo2 implements bar
    {
       // is there any legal way (syntactically or in ABC)
       // to specify it this way? ie in the "bar" namespace
       bar function barfunc();
    }

If foo2 is legal (or even currently possible in existing ABC, regardless of legality) then the verifier trick wouldn't work.       

 

Comment 11

[Steven Johnson]

Hey Tommy, I think that in addition to

    I(inst).foo();
    inst.I::foo();

there's also

    (inst as I).foo()

and of course all the variations on specifying the ns or name at runtime vs compiletime, but those are probably solvable.

Comment 12

[Steven Johnson]

Actually, thinking on this further, I don't think we can catch all the cases in the Verifier; consider these perfectly legal constructs:

   var i = I;
   var f = "foo";

   i(inst)[f]();     
   (inst as i)[f](); 

I don't think the Verifier can reliably identify i as an Interface in this case, and even if it could, it couldn't verify that f was a valid method name of it. So I'm abandoning the Verifier idea for now unless someone suggests otherwise.

Comment 13

[Jeff Dyer]

I'm trying to get my head around this bug and am having trouble finding the test case. Can someone point me to it? Thanks.

Jd

Comment 14

[Steven Johnson]

jeff: look at test/acceptance/as3/Definitions/Interfaces/bug-438830.as for an example.

everyone else: more work on the symtable rep is definitely in order, but another huge culprit is the abomination known as QCache. (And I can say that 'cuz I wrote the thing). 

Switching to a super-simpleminded linked-list LRU cache (access == move to head of list) gives me much much better results with a cache size of 32 than QCache gave me with 128. (I'm getting a hit rate of over 99.2% now.) It also shaves over a second of start time from my complex Flex app. 

I'm going to clean up the simpleminded cache and offer a patch Tuesday morning, but probably more work on cache smarts could be done that could further minimize the speed pain.

Comment 15

[Steven Johnson]

Attachment: Patch

Additional patch to replace QCache with a more conventional linked-list caching approach. Gives much better performance with a smaller cache size, especially for complex class hierarchies like Flex.

Comment 16

[Steven Johnson]

Note, patch above is additive to the previous patch.

Comment 17

[Steven Johnson]

pushed as changeset:   435:6acea6f9edea

Comment 18

[Edwin Smith]

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

* extra //#include in TraitsData.cpp

* in QCache, exit(-1) after Assert(0) seems dangerous, adds link on otherwise unreferenced crt function

* massive diff in vcproj files, line endings?

Comment 19

[Steven Johnson]

the exit(-1) was already removed on a subsequent patch.

vcproj diff is line endings.