532098 - Sampler API needs new edge

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Tommy Reilly]

The sample API needs to make a connection between a function closure and its activation so the heap graph makes sense, as it is activation objects show up as gc roots since the root definition FB uses is "no one else points at me"

Comment 1

[Tommy Reilly]

psuedo code solution:

ScriptObject o
if(o is Function)
{
   MethodEnv *env = (FunctionScriptObject*)o->_call;
   ScopeChain *sc = env->scope();
   for(int i=scope.getSize()-1; i <= 0; i--) {
      if(sc.getScope(i)->vtable() == env->getActivationVTable()) {
         addFakeHeapGraphEdge(sc.getScope(i), o, ".activation");
         break;
      }
   }
}

Comment 2

[Tommy Reilly]

Attachment: exposes captured activations in DEBUGGER builds

Not happy with the extra trait in Function but otherwise this works.  Is that okay since its private?

Comment 3

[Edwin Smith]

Is there a reason the sampler cannot "see" the real edges from Function -> MethodEnv -> ScopeChain -> Activation?

Comment 4

[Tommy Reilly]

The sampler only sees edges at the AS level not the C++ level, this patch surfaces the hidden C++ edges to AS.

Comment 5

[Edwin Smith]

At the AS level, there is such a thing as lexical scope, and objects that are only reachable as lexical scope.  The sampler needs a way to express that.  Lexical scope is a true aspect of ActionScript, not a magic behind the scenes C++ thing, and not like a root like things on the stack.  Is it feasible?  Sounds like the sampler can express roots, so perhaps it can express lexically scoped objects too?  (every function has a set of objects in its lexical scope).

Another example would be with objects:

  with (new C()) {
    return function () {
       /* C is alive as long as this function instance is */
    }
  }

Comment 6

[Tommy Reilly]

Attachment: updated patch with no Function traits pollution

Comment 7

[Edwin Smith]

Comment on attachment 420736 [details] [diff] [review]
updated patch with no Function traits pollution

As an afterthought, it might have been better to use a Vector<Object> instead of Array.  But, I don't see anything obviously wrong with the patch as-is.

Comment 8

[Tommy Reilly]

changeset:   3487:64c09a458735

Comment 9

[Chris Peyer]

Engineering work item.  Marking as verified.

Comment 10

[Mike Morearty]

It seems wrong to me that getLexicalScopes() can return null instead of an empty array in some cases:

	ArrayObject* MethodEnv::getLexicalScopes()
	{
		if(_scope->getSize() == 0)
			return NULL;
	...

Wouldn't it make more sense to return an empty array?  (For that matter, can _scope->getSize() actually return zero?)

Comment 11

[Lars T Hansen]

(In reply to comment #10)
> It seems wrong to me that getLexicalScopes() can return null instead of an
> empty array in some cases:
> 
>     ArrayObject* MethodEnv::getLexicalScopes()
>     {
>         if(_scope->getSize() == 0)
>             return NULL;
>     ...
> 
> Wouldn't it make more sense to return an empty array?  (For that matter, can
> _scope->getSize() actually return zero?)

Agree this seems dodgy, could be an optimization for a common case (avoids allocation) and not sloppy coding though.