Compute an incremental patch to JavaScript code.

Compute an incremental patch to JavaScript code.

R=johnniwinther@google.com

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

[ahe, 2014-10-07 13:07:50]

ahe@google.com changed reviewers:
+ johnniwinther@google.com

[ahe, 2014-10-07 13:07:51]

I have some manual testing of this. In the next CL, I'll develop an automated
test.

[ahe, 2014-10-07 15:25:44]

Uploaded a new patch set which works around the bug in
JsBuilder/TemplateManager.

Also added option to test minified output.

[ahe, 2014-10-07 15:43:59]

Added option to make testing reproducible by others.

How to use:


1. Compile a simple program such as hello-world:

DART_VM_OPTIONS=-DDART2JS_EXPERIMENTAL_INCREMENTAL_SUPPORT=true
./sdk/bin/dart2js --disable-type-inference --minify hello.dart

(On Windows you have to define the environment variable "DART_VM_OPTIONS" to
have the value "-DDART2JS_EXPERIMENTAL_INCREMENTAL_SUPPORT=true". Not sure how
many quotes are needed.)

2. Create an empty file called patch.js.

3. Run the program:

./third_party/d8/macos/d8 patch.js site/try/poi/patcher.js out.js

(Depending on your OS, substitute ./third_party/d8/linux/d8 or
third_party/d8/windows/d8.exe for ./third_party/d8/macos/d8.)

4. Copy hello.dart to hello.dart.1.dart, modify the copy (simple modifications
only, for example, change the greeting).

5. Run poi.dart:

./sdk/bin/dart -DPRINT_SCOPE_INFO=false -pp/ site/try/poi/poi.dart --minify
--compile --simulate-mutation hello.dart 10 10

It will output something like:

--->>>                                                         1371.160ms
Simple modification of function(main) detected
Analyzing changes and updating elements took                      8.274ms
var $dart_patch = "(function(f) {\n  init.globalFunctions.main = G.main =
f;\n})(...);\n";

--->>>                                                           19.109ms

6. Create the patch by copying the line containing "var $dart_patch" into the
file patch.js.

7. Repeat the the d8 command. Notice that the behavior changed.

[floitsch, 2014-10-07 15:56:57]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2014-10-07 15:56:58]

DBC.

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:233: // names in
updateScope don't shadow global names.
The only temporary variable updateScope has is the one that is needed to pass
the value along.

Couldn't you just write?
return js.statement('#.# = #.# = #;',
    [ globalFunctionsAccess, name, globalObject, name, function ]);

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:235: [new
jsAst.Parameter('f')], new jsAst.Block.empty());
does the following work?

jsAst.Fun = js('function(f) { #; }, updateGlobalFunction);

or even:

return js.statement('!function(f) { #.# = #.# = f; }(#);',
    [ globalFunctionsAccess, name, globalObject, name, function ]);

[ahe, 2014-10-07 16:32:40]

Florian, thank you for taking a look.

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:233: // names in
updateScope don't shadow global names.
On 2014/10/07 15:56:58, floitsch wrote:
> The only temporary variable updateScope has is the one that is needed to pass
> the value along.
> 
> Couldn't you just write?
> return js.statement('#.# = #.# = #;',
>     [ globalFunctionsAccess, name, globalObject, name, function ]);

Yes, but I know I want to add at least one more statement to support tearOffs:

js.statement('#.main$closure().call$2 = f', globalObject);

This statement is conditional based on whether or not element can be torn off.

As I start supporting instance members, I expect this become more conditional.

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:235: [new
jsAst.Parameter('f')], new jsAst.Block.empty());
On 2014/10/07 15:56:58, floitsch wrote:
> does the following work?
> 
> jsAst.Fun = js('function(f) { #; }, updateGlobalFunction);
> 
> or even:
> 
> return js.statement('!function(f) { #.# = #.# = f; }(#);',
>     [ globalFunctionsAccess, name, globalObject, name, function ]);

Yes. But doesn't scale to multiple statements.  I tried this:

var jsAst.Block block = ...;
...
jsAst.Fun = js('function(f) #, block);

That doesn't work, and I'm not inclined to modify the parser to support this :-)

[floitsch, 2014-10-07 17:48:38]

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:233: // names in
updateScope don't shadow global names.
On 2014/10/07 16:32:40, ahe wrote:
> On 2014/10/07 15:56:58, floitsch wrote:
> > The only temporary variable updateScope has is the one that is needed to
pass
> > the value along.
> > 
> > Couldn't you just write?
> > return js.statement('#.# = #.# = #;',
> >     [ globalFunctionsAccess, name, globalObject, name, function ]);
> 
> Yes, but I know I want to add at least one more statement to support tearOffs:
> 
> js.statement('#.main$closure().call$2 = f', globalObject);
> 
> This statement is conditional based on whether or not element can be torn off.
> 
> As I start supporting instance members, I expect this become more conditional.

Maybe you could write it with conditions:
if (#) #.main$closure().call$2 = #.# 
[ hasTearoff, X, globalObject, name ]

The JS pretty-printer removes conditions with bools.

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:235: [new
jsAst.Parameter('f')], new jsAst.Block.empty());
On 2014/10/07 16:32:40, ahe wrote:
> On 2014/10/07 15:56:58, floitsch wrote:
> > does the following work?
> > 
> > jsAst.Fun = js('function(f) { #; }, updateGlobalFunction);
> > 
> > or even:
> > 
> > return js.statement('!function(f) { #.# = #.# = f; }(#);',
> >     [ globalFunctionsAccess, name, globalObject, name, function ]);
> 
> Yes. But doesn't scale to multiple statements.  I tried this:
> 
> var jsAst.Block block = ...;
> ...
> jsAst.Fun = js('function(f) #, block);
> 
> That doesn't work, and I'm not inclined to modify the parser to support this
:-)

I think you don't even need a block:

js('function(f) { #; }', [[list of statements]])

If that doesn't work, I can look up how it should be done. I'm sure there is an
easy way.

[sra1, 2014-10-07 18:03:04]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2014-10-07 18:03:06]

Templates can do everything you want to do.

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:206:
updates.statements.add(computeMemberUpdateJs(element));
Construct ASTs functionally.
Don't modify constructed ASTs.
They are permitted to share structure and use non-growable lists (which saves
lots of space).
Construct the list first, or use a template to spread an Iterable into a block:

jsAst.Block updates = js.statement('{ # }',
   compiler.enqueuer.codegen.newlyEnqueuedElements.map(computeMemberUpdateJs));

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:233: // names in
updateScope don't shadow global names.
On 2014/10/07 16:32:40, ahe wrote:
> On 2014/10/07 15:56:58, floitsch wrote:
> > The only temporary variable updateScope has is the one that is needed to
pass
> > the value along.
> > 
> > Couldn't you just write?
> > return js.statement('#.# = #.# = #;',
> >     [ globalFunctionsAccess, name, globalObject, name, function ]);
> 
> Yes, but I know I want to add at least one more statement to support tearOffs:
> 
> js.statement('#.main$closure().call$2 = f', globalObject);
> 
> This statement is conditional based on whether or not element can be torn off.
> 
> As I start supporting instance members, I expect this become more conditional.

You can conditionally generate code with templates:

https://code.google.com/p/dart/source/browse/trunk/dart/tests/compiler/dart2j...

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:235: [new
jsAst.Parameter('f')], new jsAst.Block.empty());
On 2014/10/07 16:32:40, ahe wrote:
> On 2014/10/07 15:56:58, floitsch wrote:
> > does the following work?
> > 
> > jsAst.Fun = js('function(f) { #; }, updateGlobalFunction);
> > 
> > or even:
> > 
> > return js.statement('!function(f) { #.# = #.# = f; }(#);',
> >     [ globalFunctionsAccess, name, globalObject, name, function ]);
> 
> Yes. But doesn't scale to multiple statements.  I tried this:
> 
> var jsAst.Block block = ...;
> ...
> jsAst.Fun = js('function(f) #, block);
> 
> That doesn't work, and I'm not inclined to modify the parser to support this
:-)

It does work, with a minor tweak.

Blocks (and iterables) are spliced into blocks on substitution, e.g.:
https://code.google.com/p/dart/source/browse/trunk/dart/tests/compiler/dart2j...

So:

   js('function (f) { # }', block);

The block will be spliced. Empty blocks and lists are spliced without extra
empty statements.

Given that in future you can use splicing (of a list that is built
conditionally) and / or conditionally generation with template if-statements,
you can write the meat of this function as:


return js.statement(
    '''(function(f) {
          #.# = #.# = f;
          if (#) #.main$closure().call$2 = f';
        })(#);
    ''',
    [globalFunctionsAccess, name, globalObject, name,
     isTearOff, globalObject,
     function]);
  

When I created the templates I intended the templates to be primary AST API.
Lets avoid manual constructors if at all possible.

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:236:
updateScope.body.statements.add(updateGlobalFunction);
Ditto: Construct ASTs functionally.

[ahe, 2014-10-08 12:09:45]

Thank you, Stephen and Florian for taking a look.

Johnni, please take a look at the updated patch set.

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:206:
updates.statements.add(computeMemberUpdateJs(element));
On 2014/10/07 18:03:06, sra1 wrote:
> Construct ASTs functionally.
> Don't modify constructed ASTs.
> They are permitted to share structure and use non-growable lists (which saves
> lots of space).
> Construct the list first, or use a template to spread an Iterable into a
block:
> 
> jsAst.Block updates = js.statement('{ # }',
>   
compiler.enqueuer.codegen.newlyEnqueuedElements.map(computeMemberUpdateJs));

I'm doing something like that.

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:233: // names in
updateScope don't shadow global names.
On 2014/10/07 18:03:06, sra1 wrote:
> On 2014/10/07 16:32:40, ahe wrote:
> > On 2014/10/07 15:56:58, floitsch wrote:
> > > The only temporary variable updateScope has is the one that is needed to
> pass
> > > the value along.
> > > 
> > > Couldn't you just write?
> > > return js.statement('#.# = #.# = #;',
> > >     [ globalFunctionsAccess, name, globalObject, name, function ]);
> > 
> > Yes, but I know I want to add at least one more statement to support
tearOffs:
> > 
> > js.statement('#.main$closure().call$2 = f', globalObject);
> > 
> > This statement is conditional based on whether or not element can be torn
off.
> > 
> > As I start supporting instance members, I expect this become more
conditional.
> 
> You can conditionally generate code with templates:
> 
>
https://code.google.com/p/dart/source/browse/trunk/dart/tests/compiler/dart2j...
> 
> 

I think I'll avoid templates with conditionals in this situation. It feels like
adding an extra level of interpretation (slow) to a boolean I already have
computed, and I'm not sure the code is more readable.

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:235: [new
jsAst.Parameter('f')], new jsAst.Block.empty());
On 2014/10/07 18:03:06, sra1 wrote:
> return js.statement(
>     '''(function(f) {
>           #.# = #.# = f;
>           if (#) #.main$closure().call$2 = f';
>         })(#);
>     ''',
>     [globalFunctionsAccess, name, globalObject, name,
>      isTearOff, globalObject,
>      function]);

That's pretty nifty, but I feel that this is more readable:

[
  js.statement('#.# = #.# = f', [globalFunctionsAccess, name, globalObject,
name]),
  js.statement('if (#) #.#.# = f', [isTearOff, globalObject, closureName,
callSelector]),
]

What I like about that is the proximity of # to its value.

And I think this is easier to read:

if (isTearOff) {
  statements.add(js.statement('#.#().# = f', [globalObject, closureName,
callSelector]));
}

I think it is easier to read because there's no mental overhead to match # to
"isTearOff".

https://codereview.chromium.org/636903002/diff/60001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:236:
updateScope.body.statements.add(updateGlobalFunction);
On 2014/10/07 18:03:06, sra1 wrote:
> Ditto: Construct ASTs functionally.

I don't know what you mean by that.

[Johnni Winther, 2014-10-08 13:21:50]

lgtm

https://codereview.chromium.org/636903002/diff/80001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/636903002/diff/80001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:224: jsAst.Node
globalObject = compiler.backend.namer.globalObjectFor(element);
globalObject should be a String.

[ahe, 2014-10-09 12:57:20]

Committed patchset #6 (id:100001) manually as 41011 (presubmit successful).

[ahe, 2014-10-09 13:56:27]

Thank you, Johnni.

https://codereview.chromium.org/636903002/diff/80001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/636903002/diff/80001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:224: jsAst.Node
globalObject = compiler.backend.namer.globalObjectFor(element);
On 2014/10/08 13:21:50, Johnni Winther wrote:
> globalObject should be a String.

Actually, I should probably use Namer.elementAccess instead, so that's what I've
done.