RFC: Start refactoring to provide more than a single backend.

RFC: Start refactoring to provide more than a single backend.

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

[Anton Muhin, 2012-05-11 05:45:42]

Peter and Kasper,

that's RFC to discuss the general approach for embedding dart backend into
dart2js.

Does approach in this CL look reasonable to you or you envisioned something
different?

Most probably I'll move an emitter into backend as well.

[ahe, 2012-05-11 11:39:48]

LGTM, but please wait for at least one LGTM from Kasper, Florian, or Nicolas.

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

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:33: interface Backend {
This looks reasonable, but I would prefer to have a common base class instead of
an interface. In dart2js, we tend to make much fewer interfaces than is common
in Java.

One of the reasons is that we have implicit interfaces from classes.

Also, there is a bit of an allergic reaction based on our experience in dartc.

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:37: class JsBackend implements Backend
{
Personally, I prefer JavaScriptBackend instead of the abbreviation.

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:784: interceptors =
compiler.backend.builder.interceptors,
I think this will cause a type warning. compiler.backend is a Backend which
doesn't have a builder.

[ngeoffray, 2012-05-11 12:43:30]

LGTM, with Peter's comments. Welcome aboard Anton!

[floitsch, 2012-05-11 13:51:04]

I wanted to move the backend related code out of the compiler for a long time.
This is a step in the right direction so this LGTM.

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

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:33: interface Backend {
On 2012/05/11 11:39:48, ahe wrote:
> This looks reasonable, but I would prefer to have a common base class instead
of
> an interface. In dart2js, we tend to make much fewer interfaces than is common
> in Java.
> 
> One of the reasons is that we have implicit interfaces from classes.
> 
> Also, there is a bit of an allergic reaction based on our experience in dartc.

Personally I prefer an interface over an empty class. If there is something to
share, I would of course prefer a class.

[Anton Muhin, 2012-05-12 15:48:43]

Thanks a lot for both feedback and welcome.

Next iteration.  Principal change is I moved emitter into backend as well, as it
least of now it looks pretty JS-specific.

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

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:33: interface Backend {
I erred to interface for as of now I'd rather move any common code into compiler
than backend.

Again, you all should know this stuff better so I'd introduce a base class if
you think it's better approach.

On 2012/05/11 13:51:04, floitsch wrote:
> On 2012/05/11 11:39:48, ahe wrote:
> > This looks reasonable, but I would prefer to have a common base class
instead
> of
> > an interface. In dart2js, we tend to make much fewer interfaces than is
common
> > in Java.
> > 
> > One of the reasons is that we have implicit interfaces from classes.
> > 
> > Also, there is a bit of an allergic reaction based on our experience in
dartc.
> 
> Personally I prefer an interface over an empty class. If there is something to
> share, I would of course prefer a class.

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:37: class JsBackend implements Backend
{
On 2012/05/11 11:39:48, ahe wrote:
> Personally, I prefer JavaScriptBackend instead of the abbreviation.

Done.

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:784: interceptors =
compiler.backend.builder.interceptors,
On 2012/05/11 11:39:48, ahe wrote:
> I think this will cause a type warning. compiler.backend is a Backend which
> doesn't have a builder.

I am sorry I probably missed something, what kind of warning it will cause? 
While being run, it shouldn't be an issue (I hope) as backend will have
necessary methods.  What I am missing?

[ngeoffray, 2012-05-13 17:02:47]

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:784: interceptors =
compiler.backend.builder.interceptors,
On 2012/05/12 15:48:43, antonmuhin wrote:
> On 2012/05/11 11:39:48, ahe wrote:
> > I think this will cause a type warning. compiler.backend is a Backend which
> > doesn't have a builder.
> 
> I am sorry I probably missed something, what kind of warning it will cause? 
> While being run, it shouldn't be an issue (I hope) as backend will have
> necessary methods.  What I am missing? 

It will create a static type warning, eg: "interface backend has no field named
'builder'". We want to be warning free. Indeed, at runtime, the backend object
will have a 'builder' field, but Peter is referring to compile-time.

[kasperl, 2012-05-14 06:31:27]

LGTM. I would look at caching compiler.backend.emitter etc. in a few places to
avoid these very long unwieldy names.

[ahe, 2012-05-14 08:11:35]

On 2012/05/14 06:31:27, kasperl wrote:
> LGTM. I would look at caching compiler.backend.emitter etc. in a few places to
> avoid these very long unwieldy names.

It looks to me like Backend should be an abstract class like this:

abstract class Backend {
  final Compiler compiler;
  final Emitter emitter;

  Backend(this.compiler, this.emitter);
}

[ahe, 2012-05-14 08:13:33]

On 2012/05/14 08:11:35, ahe wrote:
> On 2012/05/14 06:31:27, kasperl wrote:
> > LGTM. I would look at caching compiler.backend.emitter etc. in a few places
to
> > avoid these very long unwieldy names.
> 
> It looks to me like Backend should be an abstract class like this:
> 
> abstract class Backend {
>   final Compiler compiler;
>   final Emitter emitter;
> 
>   Backend(this.compiler, this.emitter);
> }

Sorry, the field compiler should have been builder.

[ahe, 2012-05-14 08:14:30]

Still LGTM!

https://chromiumcodereview.appspot.com/10386086/diff/8/lib/compiler/implement...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10386086/diff/8/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:30: class JavaScriptBackend implements
Backend {
Consider creating a separate library for this class.

[Anton Muhin, 2012-05-14 14:36:45]

PTAL

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:784: interceptors =
compiler.backend.builder.interceptors,
Thanks a lot for explanation, Nicolas!

Q: how do I test my changes?  Currently I use:


./tools/build.py --mode=release create_sdk && ./tools/test.py --mode=release
--compiler=dart2js --runtime=d8 --time --use-sdk --report --shards=4 --shard=1

but that didn't show any warning.

I remember you used to have a presubmit script.

Any advice?

And to solve the problem I try to pass proper instances, e.g. JavaScriptBackend,
etc.
On 2012/05/13 17:02:47, ngeoffray wrote:
> On 2012/05/12 15:48:43, antonmuhin wrote:
> > On 2012/05/11 11:39:48, ahe wrote:
> > > I think this will cause a type warning. compiler.backend is a Backend
which
> > > doesn't have a builder.
> > 
> > I am sorry I probably missed something, what kind of warning it will cause? 
> > While being run, it shouldn't be an issue (I hope) as backend will have
> > necessary methods.  What I am missing? 
> 
> It will create a static type warning, eg: "interface backend has no field
named
> 'builder'". We want to be warning free. Indeed, at runtime, the backend object
> will have a 'builder' field, but Peter is referring to compile-time.

https://chromiumcodereview.appspot.com/10386086/diff/8/lib/compiler/implement...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10386086/diff/8/lib/compiler/implement...
lib/compiler/implementation/compiler.dart:30: class JavaScriptBackend implements
Backend {
On 2012/05/14 08:14:30, ahe wrote:
> Consider creating a separate library for this class.

What would be your recommendation where to place it?  into
compiler/implementation or compiler/implementation/ssa?

And is it worth creating a separate lib?  Maybe just a separate file?

https://chromiumcodereview.appspot.com/10386086/diff/6006/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10386086/diff/6006/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:781: emitCallStubForGetter(member,
selectors, defineInstanceMember);
might have been an oversight while moving code around, but still, is it ok?

[floitsch, 2012-05-14 15:10:29]

https://chromiumcodereview.appspot.com/10386086/diff/6006/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10386086/diff/6006/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:781: emitCallStubForGetter(member,
selectors, defineInstanceMember);
On 2012/05/14 14:36:45, antonmuhin wrote:
> might have been an oversight while moving code around, but still, is it ok?

LGTM.

[floitsch, 2012-05-14 15:12:05]

On 2012/05/14 14:36:45, antonmuhin wrote:
> PTAL
> 
>
https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
> File lib/compiler/implementation/ssa/builder.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
> lib/compiler/implementation/ssa/builder.dart:784: interceptors =
> compiler.backend.builder.interceptors,
> Thanks a lot for explanation, Nicolas!
> 
> Q: how do I test my changes?  Currently I use:
> 
> 
> ./tools/build.py --mode=release create_sdk && ./tools/test.py --mode=release
> --compiler=dart2js --runtime=d8 --time --use-sdk --report --shards=4 --shard=1
> 
> but that didn't show any warning.
If you don't use the DartEditor you will probably get those wrong from time to
time. One solution would be to run dartc as part of the tests or as a presubmit,
but that's not yet done.

> 
> I remember you used to have a presubmit script.
> 
> Any advice?
> 
> And to solve the problem I try to pass proper instances, e.g.
JavaScriptBackend,
> etc.
> On 2012/05/13 17:02:47, ngeoffray wrote:
> > On 2012/05/12 15:48:43, antonmuhin wrote:
> > > On 2012/05/11 11:39:48, ahe wrote:
> > > > I think this will cause a type warning. compiler.backend is a Backend
> which
> > > > doesn't have a builder.
> > > 
> > > I am sorry I probably missed something, what kind of warning it will
cause? 
> > > While being run, it shouldn't be an issue (I hope) as backend will have
> > > necessary methods.  What I am missing? 
> > 
> > It will create a static type warning, eg: "interface backend has no field
> named
> > 'builder'". We want to be warning free. Indeed, at runtime, the backend
object
> > will have a 'builder' field, but Peter is referring to compile-time.
> 
>
https://chromiumcodereview.appspot.com/10386086/diff/8/lib/compiler/implement...
> File lib/compiler/implementation/compiler.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10386086/diff/8/lib/compiler/implement...
> lib/compiler/implementation/compiler.dart:30: class JavaScriptBackend
implements
> Backend {
> On 2012/05/14 08:14:30, ahe wrote:
> > Consider creating a separate library for this class.
> 
> What would be your recommendation where to place it?  into
> compiler/implementation or compiler/implementation/ssa?
> 
> And is it worth creating a separate lib?  Maybe just a separate file?
> 
>
https://chromiumcodereview.appspot.com/10386086/diff/6006/lib/compiler/implem...
> File lib/compiler/implementation/emitter.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10386086/diff/6006/lib/compiler/implem...
> lib/compiler/implementation/emitter.dart:781: emitCallStubForGetter(member,
> selectors, defineInstanceMember);
> might have been an oversight while moving code around, but still, is it ok?

[ahe, 2012-05-14 15:56:41]

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10386086/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:784: interceptors =
compiler.backend.builder.interceptors,
On 2012/05/14 14:36:45, antonmuhin wrote:
> 
> ./tools/build.py --mode=release create_sdk && ./tools/test.py --mode=release
> --compiler=dart2js --runtime=d8 --time --use-sdk --report --shards=4 --shard=1

If you want to make sure you run all the tests, you should remove the "shard"
related options. If you just want to run a quarter of the tests, then the shard
options are fine. When making changes to the infrastructure (like you are)
running only a subset of tests is probably sufficient.

Also, if you remove --use-sdk, you don't have to run build.py.

You should also make sure that the unit tests still run. These are likely to
break when refactoring:

./tools/test.py -mrelease -rvm leg

The unit tests must be run on the VM because dart2js is not self-hosted, and
because they are *unit* tests, not *integration* tests. Side note: we should
probably start looking into getting the unit tests running on dart2js as well,
but I don't think that can replace running them on the VM.

[ahe, 2012-05-14 16:11:15]

Please don't forget to remove "RFC" from the description before submitting.