Interface override dispatch doesn't work

RESOLVED FIXED

Status

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: treilly, Assigned: stejohns)

Tracking

Details

Attachments

(3 attachments)

(Reporter)

Description

10 years ago
See attached test case.
(Reporter)

Comment 1

10 years ago
Created attachment 324445 [details] [diff] [review]
fixes attached test case

Would another way to tackle this to be able to change the look at verify time?
Attachment #324445 - Flags: review?(stejohns)
(Reporter)

Comment 2

10 years ago
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!

(Reporter)

Updated

10 years ago
Attachment #324445 - Flags: review?(edwsmith)
(Assignee)

Updated

10 years ago
Attachment #324445 - Flags: review?(stejohns) → review+
(Assignee)

Comment 3

10 years ago
Need to add a new sanity test for this to acceptance tests
(Reporter)

Comment 4

10 years ago
pushed 
(Reporter)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Attachment #324445 - Flags: review?(edwsmith)
(Reporter)

Updated

10 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 5

10 years ago
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".
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
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.)
(Reporter)

Comment 8

10 years ago
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?  
(Assignee)

Comment 9

10 years ago
Created attachment 326346 [details] [diff] [review]
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)
Attachment #326346 - Flags: review?(edwsmith)
(Assignee)

Updated

10 years ago
Attachment #326346 - Flags: review?(treilly)
(Assignee)

Comment 10

10 years ago
> 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.       

 

(Assignee)

Comment 11

10 years ago
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.
(Assignee)

Comment 12

10 years ago
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

10 years ago
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
(Assignee)

Comment 14

10 years ago
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.
(Assignee)

Comment 15

10 years ago
Created attachment 326514 [details] [diff] [review]
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.
Attachment #326514 - Flags: review?(edwsmith)
(Assignee)

Comment 16

10 years ago
Note, patch above is additive to the previous patch.
(Reporter)

Updated

10 years ago
Attachment #326346 - Flags: review?(treilly) → review+
(Assignee)

Updated

10 years ago
Attachment #326514 - Flags: review?(treilly)
(Reporter)

Updated

10 years ago
Attachment #326514 - Flags: review?(treilly) → review+
(Assignee)

Comment 17

10 years ago
pushed as changeset:   435:6acea6f9edea
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 18

10 years ago
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?

Updated

10 years ago
Attachment #326346 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 19

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

vcproj diff is line endings.

Updated

10 years ago
Attachment #326514 - Flags: review?(edwsmith) → review+
You need to log in before you can comment on or make changes to this bug.