Refactor common logic of SsaBuilder.visitSend.

Refactor common logic of SsaBuilder.visitSend.

	lib/compiler/implementation/dart_backend.dart

R=ahe@google.com,smok@google.com

Committed: https://code.google.com/p/dart/source/detail?r=8316

[Anton Muhin, 2012-05-25 11:11:58]

https://chromiumcodereview.appspot.com/10441044/diff/1/lib/compiler/implement...
File lib/compiler/implementation/tree/tree.dart (right):

https://chromiumcodereview.appspot.com/10441044/diff/1/lib/compiler/implement...
lib/compiler/implementation/tree/tree.dart:10: #import('../leg.dart');
if we don't want those cyclic deps, I can move analyse code into leg lib, e.g.,
directly into Compiler or around.

[ahe, 2012-05-29 10:24:00]

https://chromiumcodereview.appspot.com/10441044/diff/1/lib/compiler/implement...
File lib/compiler/implementation/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/10441044/diff/1/lib/compiler/implement...
lib/compiler/implementation/tree/nodes.dart:297: analyse([
It concerns that I don't see how this can avoid creating a whole bunch of
objects (closures). You cannot afford that, we do it anyways in a lot of cases,
but API like this (if feasible) should be designed to avoid creating objects.
Based on my experience in a similar compiler, I believe that keeping a lid on
the number of little objects created is key to performance.

I have an alternative suggestion.

Create an interface like this:

interface ResolvedSendVisitor<R> extends Visitor<R> {
  R visitSuperRead(Send node);
  R visitSuperSend(Send node);
  R visitOperatorSend(Send node);
  ...
}

And this method should be called 'resolvedAccept' or something similar.

[Anton Muhin, 2012-05-31 18:56:38]

https://chromiumcodereview.appspot.com/10441044/diff/1/lib/compiler/implement...
File lib/compiler/implementation/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/10441044/diff/1/lib/compiler/implement...
lib/compiler/implementation/tree/nodes.dart:297: analyse([
On 2012/05/29 10:24:00, ahe wrote:
> It concerns that I don't see how this can avoid creating a whole bunch of
> objects (closures). You cannot afford that, we do it anyways in a lot of
cases,
> but API like this (if feasible) should be designed to avoid creating objects.
> Based on my experience in a similar compiler, I believe that keeping a lid on
> the number of little objects created is key to performance.
> 
> I have an alternative suggestion.
> 
> Create an interface like this:
> 
> interface ResolvedSendVisitor<R> extends Visitor<R> {
>   R visitSuperRead(Send node);
>   R visitSuperSend(Send node);
>   R visitOperatorSend(Send node);
>   ...
> }
> 
> And this method should be called 'resolvedAccept' or something similar.

Done.

[ahe, 2012-06-01 13:50:34]

I don't think the analyze method belongs as a method on Send. Instead, it feels
like a method that should be in a visitor, not the AST node. This way, we can
keep the AST nodes unaware of the elements.

https://chromiumcodereview.appspot.com/10441044/diff/4002/lib/compiler/implem...
File lib/compiler/implementation/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/10441044/diff/4002/lib/compiler/implem...
lib/compiler/implementation/tree/nodes.dart:297: analyse([
To be perfectly clear: I don't like this method.

What I don't like is the following:

* the name of the method.

* optional arguments

* the argument methodInterceptionEnabled (it doesn't belong here).

https://chromiumcodereview.appspot.com/10441044/diff/4002/lib/compiler/implem...
File lib/compiler/implementation/tree/tree.dart (right):

https://chromiumcodereview.appspot.com/10441044/diff/4002/lib/compiler/implem...
lib/compiler/implementation/tree/tree.dart:11:
#import('../elements/elements.dart');
I don't like adding these imports.

[Anton Muhin, 2012-06-01 14:07:06]

Neither do I like additional deps.

What kind of visitor you're referring?  If it's node visitor, it's still in tree
package and we have the same problem.

I can easily lift this logic into compiler itself.  If it's ok, what would be
the recommend place?  or a fresh file?

https://chromiumcodereview.appspot.com/10441044/diff/4002/lib/compiler/implem...
File lib/compiler/implementation/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/10441044/diff/4002/lib/compiler/implem...
lib/compiler/implementation/tree/nodes.dart:297: analyse([
On 2012/06/01 13:50:34, ahe wrote:
> To be perfectly clear: I don't like this method.
> 
> What I don't like is the following:
> 
> * the name of the method.

What will you like more?  analyseSubtype?  subtypeAccess?

> 
> * optional arguments

Sorry, those a remnants of passing many closures, with new interface they may go
away.

> * the argument methodInterceptionEnabled (it doesn't belong here).

That irritates me too, but do you have any better idea? One option would be to
move methodIntercepted into SSA builder as if methodInterception is false:
visitDynamicSend/visitForeignSend abd staticSend would check if node.isOperator
and methodInterceptionEnable and delegate to visitOperatorSend, wdyt?

[ahe, 2012-06-04 14:38:52]

On 2012/06/01 14:07:06, antonmuhin wrote:
> Neither do I like additional deps.
> 
> What kind of visitor you're referring?  If it's node visitor, it's still in
tree
> package and we have the same problem.

I would create an abstract class that implements visitor. Something like:

class ResolvedVisitor<R> implements Visitor<R> {
  final TreeElements elements;

  ResolvedVisitor(this.elements);

  R visitSend(Send node) {
    // Something along the lines as what you have in node.analyze,
    // but without methodInterceptionEnabled, instead, keep this logic
    // in implementors of visitOperatorSend.
    // Somethings along the lines of:
    if (node.isSuperCall) {
      return visitSuperSend(node);
    } else if (node.isOperator) {
      return visitOperatorSend(node);
    } else if (node.isPropertyAccess) {
    ...
    } else {
      return visitRegularSend(node);
    }
  }

  abstract R visitOperatorSend(Send node);
  abstract R visitSuperSend(Send node);
  abstract R visit...
  ...
}

And in ssa/builder:

  visitOperatorSend(Send node) {
    if (!methodInterceptionEnabled) return visitRegularSend(node);
  }

This could be defined in a new file in dart/lib/compiler/implementation.

> 
> I can easily lift this logic into compiler itself.  If it's ok, what would be
> the recommend place?  or a fresh file?
> 
>
https://chromiumcodereview.appspot.com/10441044/diff/4002/lib/compiler/implem...
> File lib/compiler/implementation/tree/nodes.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10441044/diff/4002/lib/compiler/implem...
> lib/compiler/implementation/tree/nodes.dart:297: analyse([
> On 2012/06/01 13:50:34, ahe wrote:
> > To be perfectly clear: I don't like this method.
> > 
> > What I don't like is the following:
> > 
> > * the name of the method.
> 
> What will you like more?  analyseSubtype?  subtypeAccess?
> 
> > 
> > * optional arguments
> 
> Sorry, those a remnants of passing many closures, with new interface they may
go
> away.
> 
> > * the argument methodInterceptionEnabled (it doesn't belong here).
> 
> That irritates me too, but do you have any better idea? One option would be to
> move methodIntercepted into SSA builder as if methodInterception is false:
> visitDynamicSend/visitForeignSend abd staticSend would check if
node.isOperator
> and methodInterceptionEnable and delegate to visitOperatorSend, wdyt?

[Anton Muhin, 2012-06-04 18:29:00]

PTAL

[ahe, 2012-06-06 09:04:03]

LGTM!