Improve precision of JS printer callbacks (2nd try)

Improve precision of JS printer callbacks (2nd try)

BUG=
R=floitsch@google.com, sra@google.com

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

[Johnni Winther, 2015-04-14 13:01:25]

johnniwinther@google.com changed reviewers:
+ floitsch@google.com, sra@google.com

[floitsch, 2015-04-14 15:09:31]

LGTM, but wait for Stephen.
(And I only skimmed a few parts).

https://codereview.chromium.org/1081313003/diff/20001/pkg/compiler/lib/src/du...
File pkg/compiler/lib/src/dump_info.dart (right):

https://codereview.chromium.org/1081313003/diff/20001/pkg/compiler/lib/src/du...
pkg/compiler/lib/src/dump_info.dart:486: void recordAstSize(jsAst.Node code, int
size) {
I would rename the variable to "node" since it reads better: if
(isTracking(node)) { ...

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
File pkg/js_ast/lib/src/printer.dart (right):

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:220:
//print('currentNode.node=${currentNode.node} node=$node');
debugprint.

[sra1, 2015-04-14 19:14:00]

LGTM provided the O(N^2) issue is fixed for a+1+1+1....+1

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
File pkg/js_ast/lib/src/printer.dart (right):

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:191: indent(); out(str);
two lines

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:605: out(' ', isWhitespace: true); out('async');
multiple lines

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:1400: void enterNode(JavaScriptPrintingContext
context, int position) {
Perhaps 'addToNode' is a better name.
On an If node it is called for 'if' '(' ')' directly, and indirectly via the
parent chain of the condition expression for those fragments.

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:1402: parent.enterNode(context, position);
This multiplies the cost of printing by the tree height.
Can you see the cost in the emitter time?
Even if it does not show up, I think we need to fix this.

1. Can this test be moved under 'startPosition == null'? Adding a subsequent
fragment to a child node is also adding a non-first fragment to the parent.

2. Can we create nodes that know they are already within the parent to avoid the
parent call?

[sra1, 2015-04-14 21:54:45]

https://chromiumcodereview.appspot.com/1081313003/diff/20001/pkg/js_ast/lib/s...
File pkg/js_ast/lib/src/printer.dart (right):

https://chromiumcodereview.appspot.com/1081313003/diff/20001/pkg/js_ast/lib/s...
pkg/js_ast/lib/src/printer.dart:46: int delimiterPosition) {}
Perhaps call this 'closingPosition' (throughout)
'delimiter' marks the boundary between separate things, i.e. is outside the
things delimited.

https://chromiumcodereview.appspot.com/1081313003/diff/20001/pkg/js_ast/lib/s...
pkg/js_ast/lib/src/printer.dart:1402: parent.enterNode(context, position);
On 2015/04/14 19:14:00, sra1 wrote:
> This multiplies the cost of printing by the tree height.
> Can you see the cost in the emitter time?

I ran this command 5 times in a row.  Emitting is slightly (~20ms) slower now.
for i in 1 2 3 4 5; do sdk/bin/dart2js -v samples-dev/swarm/swarm.dart  
--out=v1.js | grep emitter; done

Before:
Info: Code emitter took 693msec
Info: Code emitter took 699msec
Info: Code emitter took 680msec
Info: Code emitter took 695msec
Info: Code emitter took 687msec

After:
Info: Code emitter took 716msec
Info: Code emitter took 733msec
Info: Code emitter took 703msec
Info: Code emitter took 742msec
Info: Code emitter took 720msec

> Even if it does not show up, I think we need to fix this.
> 
> 1. Can this test be moved under 'startPosition == null'? Adding a subsequent
> fragment to a child node is also adding a non-first fragment to the parent.
> 
> 2. Can we create nodes that know they are already within the parent to avoid
the
> parent call?

[Johnni Winther, 2015-04-15 11:15:28]

https://codereview.chromium.org/1081313003/diff/20001/pkg/compiler/lib/src/du...
File pkg/compiler/lib/src/dump_info.dart (right):

https://codereview.chromium.org/1081313003/diff/20001/pkg/compiler/lib/src/du...
pkg/compiler/lib/src/dump_info.dart:486: void recordAstSize(jsAst.Node code, int
size) {
On 2015/04/14 15:09:31, floitsch wrote:
> I would rename the variable to "node" since it reads better: if
> (isTracking(node)) { ...

Done.

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
File pkg/js_ast/lib/src/printer.dart (right):

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:46: int delimiterPosition) {}
On 2015/04/14 21:54:44, sra1 wrote:
> Perhaps call this 'closingPosition' (throughout)
> 'delimiter' marks the boundary between separate things, i.e. is outside the
> things delimited.

Done.

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:191: indent(); out(str);
On 2015/04/14 19:14:00, sra1 wrote:
> two lines

Done.

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:220:
//print('currentNode.node=${currentNode.node} node=$node');
On 2015/04/14 15:09:31, floitsch wrote:
> debugprint.

Removed.

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:605: out(' ', isWhitespace: true); out('async');
On 2015/04/14 19:14:00, sra1 wrote:
> multiple lines

Done.

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:1400: void enterNode(JavaScriptPrintingContext
context, int position) {
On 2015/04/14 19:14:00, sra1 wrote:
> Perhaps 'addToNode' is a better name.
> On an If node it is called for 'if' '(' ')' directly, and indirectly via the
> parent chain of the condition expression for those fragments.

Done.

https://codereview.chromium.org/1081313003/diff/20001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:1402: parent.enterNode(context, position);
On 2015/04/14 19:14:00, sra1 wrote:
> This multiplies the cost of printing by the tree height.
> Can you see the cost in the emitter time?
> Even if it does not show up, I think we need to fix this.
> 
> 1. Can this test be moved under 'startPosition == null'? Adding a subsequent
> fragment to a child node is also adding a non-first fragment to the parent.
> 
> 2. Can we create nodes that know they are already within the parent to avoid
the
> parent call?

Moved the parent call under the `startPosition == null` condition. Emitter time
is immeasurable on Windows (fluctuate between 800-2500ms regardless of
strategy).

[Johnni Winther, 2015-04-15 12:09:53]

Committed patchset #3 (id:40001) manually as 45164 (presubmit successful).