Base JavaScript code position computation on JavaScript tracer.

Base JavaScript code position computation on JavaScript tracer.

Trace computation added in position_information.dart
Diff view tool added in diff_view.dart that utilizes the new computation and show coverage stats.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/e333e631f32dfcdcbf81d663f6bfadf1d5edef7f

[Johnni Winther, 2016-01-21 15:41:40]

Description was changed from

==========
Base JavaScript code position computation on JavaScript tracer.

BUG=
==========

to

==========
Base JavaScript code position computation on JavaScript tracer.

Trace computation added in position_information.dart
Diff view tool added in diff_view.dart that utilizes the new computation and
show coverage stats.
==========

[Johnni Winther, 2016-01-21 15:41:48]

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

[sra1, 2016-01-21 15:52:13]

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

[sra1, 2016-01-21 15:52:14]

DBC

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
File pkg/compiler/lib/src/io/position_information.dart (right):

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:133: /// Marker used to tag
the root nodes of non-preamble code.
non-preamble -> preamble?

[Siggi Cherem (dart-lang), 2016-01-21 18:18:35]

I'm curious - how far along is the new source-info in ssa, can we turn it on by
default?

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
File pkg/compiler/lib/src/io/position_information.dart (right):

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:107: Coverage coverage;
revert?

I couldn't find where you create a PositionSourceInformationStrategy that
records coverage. It appears that you only do the coverage processing later in
the diff tool, but not in the main compiler pipepine. 

Is the idea to add it in the future/make part of dump-info? For now I'm inclined
to leave it out, but I guess I might be missing something.

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:310: enum CodePositionKind {
nit: since this and SourcePositionKind are the same, how about unifying the
enums?

enum PositionKind { START, CLOSING, END }

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:312: CLOSING,
document what's the difference between closing and end? are they the same on
nodes that are not calls?

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:382: case
StepKind.SWITCH_EXPRESSION:
nit: replace the last 12 lines with 'default: ' ?

Or switch to an if-then-else? E.g:

   sourcePositionKind = (kind == StepKind.FUN)
       ? SourcePositionKind.CLOSING
       : (kind == StepKind.CALL)
           ? CallPosition.getSemantic...(node).sourcePositionKind
           : SourcePositionKind.START;

Or is the idea that this will diverge more in the future?

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:386: SourceLocation
sourceLocation;
optional: make this a helper `getLocation` method similar to how you added
`getPosition`?

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:471: /// The offset of the
statement containing the step.
the statement => the enclosing statement?

(I assume this will be the same offset for any subexpression inside a single
statement?)

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:474: /// The left-to-right
offset of the step.
can you clarify? Not sure what left-to-right means. Offset from the beginning of
the statement? Offset from the beginning of a line?

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:477: /// The subexpression
offset of the step.
is this relative to the beginning of the file or to the statementOffset?

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:694: offsetPosition = null;
I was expecting this to be restored to oldPosition instead.

I guess it might not matter, because on nested calls you reset the oldPosition
after visiting the arguments?

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:702: node,
getOffsetForNode(node, getSyntaxOffset(node)), StepKind.NEW);
why for `new` don't you need the same logic as for `call`? Is it because `new`
is always the starting term?

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/diff_view.dart (right):

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/diff_view.dart:5: library sourcemap.diff_view;
I only did a cursory review of this file, let me know if you want me to look in
more detail. It might be useful to have a sample output .html file to guide me
in the review process.

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/diff_view.dart:49: // source information
strategy.
is a goal also to compare the new and old source-info strategy?

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/js_tracer.dart (right):

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/js_tracer.dart:14: TraceGraph
createTraceGraph(SourceMapInfo info, Coverage coverage) {
should this function move to trace_graph.dart? Or merge that library here?

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/js_tracer.dart:140: SourceLocation
getSourceLocation(
seems you already agreed with my other suggestion ;-). Let's move this to the
source library then.

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart (right):

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart:126: class
SourceMapperWrapper implements SourceMapper {
+ dartdoc, maybe rename to something more descriptive? For instasnce:

/// A [SourceMapper] that records the source locations on each node.
class RecordingSourceMapper implements SourceMapper {
  ...
}

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart:169: class
SourceInformationSubProcess {
rename? SubProcess really makes me think that you are about to do a
Process.run/fork somewhere.

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart:183: class
SourceInformationStrategyWrapper
it took me some time to figure out what these wrappers are meant to be used for.
It appears that you are trying to record information about each element
separately? So that you can easily report results per element?

If so - maybe a possible naming scheme could be to prefix with `PerElement`?

SourceInformationProcessorWrapper => PerElementSourceInformationProcessor
SourceInformationStrategyWrapper => PerElementSourceInformationStrategy
SourceInformationSubProcess => PerElementRecorder

Since this entire file is about source-information, consider dropping the
SourceInformation piece, or abbreviating it:


PerElementProcessor
PerElementStrategy

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart:239: class FindVisitor
extends js.BaseVisitor {
+dartdoc:

/// Visitor that collects all nodes that are within a function. Used by the
[PerElementStrategy] to filter what is recorded on a [PerElementRecorder].

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart:428: class
NodeToSourceLocationsMapWrapper implements NodeToSourceLocationsMap {
nits/ideas:

- rename to PerElementNodeToSourceLocationsMap or
FilteredNodeToSourceLocationsMap
- consider making it a private class (same with any of the other ones above).
- it might be nice to put factory ctors in ModeToSourceLocationsMap for this and
the recorder, so you can write:

  new NodeToSourceLocationsMap.filter(nodes, map);

- are there words in these names we could get rid of to make it shorter, but
still be precise enough to convey what we need to say?

  It seems to me that "Source" and "NodeTo..." are not really that necessary.
Something like `LocationMap` could be descriptive enough. For example:

   NodeToSourceLocationMap => LocationMap
   NodeToSourceLocationRecorder => LocationRecorder
   NodeToSourceLocationMapWrapper => FilteredLocationMap

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/trace_graph.dart (right):

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/trace_graph.dart:33: class TraceStep<N> {
drop the generic? Make it directly use js.Node below? Or add the generic in
TraceGraph and in js_tracer?

[Johnni Winther, 2016-01-21 21:52:26]

On 2016/01/21 18:18:35, Siggi Cherem (dart-lang) wrote:
> I'm curious - how far along is the new source-info in ssa, can we turn it on
by
> default?
> 

If you try out the diff_view tool you can see the difference. As far as I
remember they are comparable but I can investigate further tomorrow.

[Johnni Winther, 2016-01-22 15:12:39]

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
File pkg/compiler/lib/src/io/position_information.dart (right):

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:107: Coverage coverage;
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> revert?
> 
> I couldn't find where you create a PositionSourceInformationStrategy that
> records coverage. It appears that you only do the coverage processing later in
> the diff tool, but not in the main compiler pipepine. 
> 
> Is the idea to add it in the future/make part of dump-info? For now I'm
inclined
> to leave it out, but I guess I might be missing something.

Reverted

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:133: /// Marker used to tag
the root nodes of non-preamble code.
On 2016/01/21 15:52:13, sra1 wrote:
> non-preamble -> preamble?

Non-preamble! In output like 

...
var dart = [["_interceptors", "dart:_interceptors",, J, {
    "^": "",
    getInterceptor: function(object) {
      return void 0;
    },
...

The preamble marker is on the Fun node for 'getInterceptor' to marker that it is
a root node of code that we want to source map.

Changed the named to SourceMappedMarker.

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:310: enum CodePositionKind {
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> nit: since this and SourcePositionKind are the same, how about unifying the
> enums?
> 
> enum PositionKind { START, CLOSING, END }

They are semantically different and might diverge. Added comments.

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:312: CLOSING,
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> document what's the difference between closing and end? are they the same on
> nodes that are not calls?

Add doc comments with examples.

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:382: case
StepKind.SWITCH_EXPRESSION:
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> nit: replace the last 12 lines with 'default: ' ?
> 
> Or switch to an if-then-else? E.g:
> 
>    sourcePositionKind = (kind == StepKind.FUN)
>        ? SourcePositionKind.CLOSING
>        : (kind == StepKind.CALL)
>            ? CallPosition.getSemantic...(node).sourcePositionKind
>            : SourcePositionKind.START;
> 
> Or is the idea that this will diverge more in the future?

I avoid using default to get the best case coverage analysis (in the face of
enums)

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:386: SourceLocation
sourceLocation;
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> optional: make this a helper `getLocation` method similar to how you added
> `getPosition`?

Done.

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:471: /// The offset of the
statement containing the step.
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> the statement => the enclosing statement?
> 
> (I assume this will be the same offset for any subexpression inside a single
> statement?)

Yes. Comment updated.

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:474: /// The left-to-right
offset of the step.
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> can you clarify? Not sure what left-to-right means. Offset from the beginning
of
> the statement? Offset from the beginning of a line?

Comments updated and examples added.

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:477: /// The subexpression
offset of the step.
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> is this relative to the beginning of the file or to the statementOffset?

Ditto.

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:694: offsetPosition = null;
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> I was expecting this to be restored to oldPosition instead.
> 
> I guess it might not matter, because on nested calls you reset the oldPosition
> after visiting the arguments?

The [offsetPosition] set be parent nodes should only be used by the first child
node, cf. the (maybe to unclear) comment on [offsetPosition].

https://codereview.chromium.org/1617083002/diff/1/pkg/compiler/lib/src/io/pos...
pkg/compiler/lib/src/io/position_information.dart:702: node,
getOffsetForNode(node, getSyntaxOffset(node)), StepKind.NEW);
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> why for `new` don't you need the same logic as for `call`? Is it because `new`
> is always the starting term?

Yes.

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/diff_view.dart (right):

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/diff_view.dart:49: // source information
strategy.
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> is a goal also to compare the new and old source-info strategy?

It is support as well.

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/js_tracer.dart (right):

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/js_tracer.dart:14: TraceGraph
createTraceGraph(SourceMapInfo info, Coverage coverage) {
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> should this function move to trace_graph.dart? Or merge that library here?

This for JS trace graphs, later we will have Dart trace graphs as well (for
quality test)

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/js_tracer.dart:140: SourceLocation
getSourceLocation(
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> seems you already agreed with my other suggestion ;-). Let's move this to the
> source library then.

Done.

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart (right):

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart:126: class
SourceMapperWrapper implements SourceMapper {
On 2016/01/21 18:18:34, Siggi Cherem (dart-lang) wrote:
> + dartdoc, maybe rename to something more descriptive? For instasnce:
> 
> /// A [SourceMapper] that records the source locations on each node.
> class RecordingSourceMapper implements SourceMapper {
>   ...
> }

Done.

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart:169: class
SourceInformationSubProcess {
On 2016/01/21 18:18:35, Siggi Cherem (dart-lang) wrote:
> rename? SubProcess really makes me think that you are about to do a
> Process.run/fork somewhere.
> 

Renamed to 'RecordedSourceInformationProcess'.

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart:183: class
SourceInformationStrategyWrapper
On 2016/01/21 18:18:35, Siggi Cherem (dart-lang) wrote:
> it took me some time to figure out what these wrappers are meant to be used
for.
> It appears that you are trying to record information about each element
> separately? So that you can easily report results per element?
> 
> If so - maybe a possible naming scheme could be to prefix with `PerElement`?
> 
> SourceInformationProcessorWrapper => PerElementSourceInformationProcessor
> SourceInformationStrategyWrapper => PerElementSourceInformationStrategy
> SourceInformationSubProcess => PerElementRecorder
> 
> Since this entire file is about source-information, consider dropping the
> SourceInformation piece, or abbreviating it:
> 
> 
> PerElementProcessor
> PerElementStrategy
> 

It's (only) per element (also) output units. Updated the names to use Record and
Recorded.

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/trace_graph.dart (right):

https://codereview.chromium.org/1617083002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/trace_graph.dart:33: class TraceStep<N> {
On 2016/01/21 18:18:35, Siggi Cherem (dart-lang) wrote:
> drop the generic? Make it directly use js.Node below? Or add the generic in
> TraceGraph and in js_tracer?

Removed for now.

[Siggi Cherem (dart-lang), 2016-01-22 19:32:58]

lgtm

https://codereview.chromium.org/1617083002/diff/20001/pkg/compiler/lib/src/io...
File pkg/compiler/lib/src/io/position_information.dart (right):

https://codereview.chromium.org/1617083002/diff/20001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:561: /// [subexpressionOffset]
where offset by the execution order are always
A few notes:
 - [subexpressionOffset] => [codeOffset]?
   Or rename codeOffset above to subexpressionOffset? (I slightly prefer
renaming)

 - grammar (not sure I follow "where offset by the execution...") Suggestion:

  "This is like [subexpressionOffset] but restricted so that the offset of each
subexpression in execution order is monotonically increasing."

https://codereview.chromium.org/1617083002/diff/20001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:571: /// Here, `baz()` is
executed before `foo()` be needs to use 'f' as its best
nit: "be needs" => "so we need to"?

https://codereview.chromium.org/1617083002/diff/20001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:574: final int
leftToRightOffset;
another idea for the name here: executionOrderOffset

[Johnni Winther, 2016-01-25 09:58:55]

https://codereview.chromium.org/1617083002/diff/20001/pkg/compiler/lib/src/io...
File pkg/compiler/lib/src/io/position_information.dart (right):

https://codereview.chromium.org/1617083002/diff/20001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:561: /// [subexpressionOffset]
where offset by the execution order are always
On 2016/01/22 19:32:58, Siggi Cherem (dart-lang) wrote:
> A few notes:
>  - [subexpressionOffset] => [codeOffset]?
>    Or rename codeOffset above to subexpressionOffset? (I slightly prefer
> renaming)
> 
>  - grammar (not sure I follow "where offset by the execution...") Suggestion:
> 
>   "This is like [subexpressionOffset] but restricted so that the offset of
each
> subexpression in execution order is monotonically increasing."

Done.

https://codereview.chromium.org/1617083002/diff/20001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:571: /// Here, `baz()` is
executed before `foo()` be needs to use 'f' as its best
On 2016/01/22 19:32:58, Siggi Cherem (dart-lang) wrote:
> nit: "be needs" => "so we need to"?

Done.

https://codereview.chromium.org/1617083002/diff/20001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:574: final int
leftToRightOffset;
On 2016/01/22 19:32:58, Siggi Cherem (dart-lang) wrote:
> another idea for the name here: executionOrderOffset

I find that equally confusing :(

[Johnni Winther, 2016-01-25 10:17:32]

Description was changed from

==========
Base JavaScript code position computation on JavaScript tracer.

Trace computation added in position_information.dart
Diff view tool added in diff_view.dart that utilizes the new computation and
show coverage stats.
==========

to

==========
Base JavaScript code position computation on JavaScript tracer.

Trace computation added in position_information.dart
Diff view tool added in diff_view.dart that utilizes the new computation and
show coverage stats.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/e333e631f32dfcdcbf81d663f6bfadf1d5edef7f
==========

[Johnni Winther, 2016-01-25 10:17:33]

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