Add --x-perf-report flag to the dartanalyzer command

Add --x-perf-report flag to the dartanalyzer command

Also added a couple of performance tags to catch time spent in the driver.

BUG=
R=brianwilkerson@google.com, paulberry@google.com

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

[skybrian, 2015-12-16 02:08:30]

skybrian@google.com changed reviewers:
+ brianwilkerson@google.com, paulberry@google.com

[skybrian, 2015-12-16 02:08:31]

If this looks useful then I will write some tests.

[Brian Wilkerson, 2015-12-16 15:08:43]

Looks good so far. I assume this is only intended for internal use (rather than
being something that users are expected to understand). It would be interesting
to add more information to the output (which might require more structure in the
output), such as the timing information for tasks.

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
File pkg/analyzer_cli/lib/src/analyzer_impl.dart (right):

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/analyzer_impl.dart:127:
errorInfos.add(context.getErrors(source));
The original code looks weird to me. Seems like we should just have one line:

errorInfos.add(context.computeErrors(source));

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/analyzer_impl.dart:132: static final PerformanceTag
_prepareErrorsTag =
Don't forget to sort and format the file before committing it.

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
File pkg/analyzer_cli/lib/src/driver.dart (right):

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/driver.dart:231: if (!firstLine) buf.writeln(",");
style nit: we always use blocks for 'if's.

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/driver.dart:236: buf.writeln("\n}");
nit: seems strange to have a newline explicitly in the string rather than

buf.writeln();
buf.writeln('}');

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
File pkg/analyzer_cli/lib/src/options.dart (right):

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/options.dart:244: ..addOption('perf-log',
I think I'd prefer to not have this be visible in the help text. Users won't
generally be able to use this information, so there's no point in advertising
it.

Either way, let's document the fact (in the help text) that the format of the
file is not stable and should not be depended on.

[Paul Berry, 2015-12-16 15:11:26]

lgtm

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
File pkg/analyzer_cli/lib/src/driver.dart (right):

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/driver.dart:227: var buf = new StringBuffer();
There's already library code available to pretty-print the JSON.  Just do this:

var encoded = new JsonEncoder.withIndent('  ').encode(json);

[skybrian, 2015-12-16 18:48:10]

re: It would be interesting to add more information to the output (which might
require more structure in the output), such as the timing information for tasks.

Yes, I was thinking I should also add environmental settings like the Dart SDK
version and options passed in, so the performance counters should move down a
level.

Also, basic statistics like number of Dart files scanned might be useful.

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
File pkg/analyzer_cli/lib/src/driver.dart (right):

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/driver.dart:227: var buf = new StringBuffer();
On 2015/12/16 15:11:26, Paul Berry wrote:
> There's already library code available to pretty-print the JSON.  Just do
this:
> 
> var encoded = new JsonEncoder.withIndent('  ').encode(json);

Thanks!

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
File pkg/analyzer_cli/lib/src/options.dart (right):

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/options.dart:244: ..addOption('perf-log',
On 2015/12/16 15:08:43, Brian Wilkerson wrote:
> I think I'd prefer to not have this be visible in the help text. Users won't
> generally be able to use this information, so there's no point in advertising
> it.
> 
> Either way, let's document the fact (in the help text) that the format of the
> file is not stable and should not be depended on.

For GWT there are compiler flags that are explicitly experimental and begin with
--X. Maybe do something like that here? So, how about
--x-perf-report=path/to/file.json

(I'm thinking of changing the name since this is more of a report than a log.)

[Brian Wilkerson, 2015-12-16 19:06:45]

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
File pkg/analyzer_cli/lib/src/options.dart (right):

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/options.dart:244: ..addOption('perf-log',
That would be fine. I just want it to be obvious to end-users that the format of
the file is not something they should depend on. Even just adding
"(Experimental)" to the description would be useful.

If we have "--x-" options (I like that idea), then they should not show up with
the normal options (but perhaps someday we could display them using a "-v"
flag).

[skybrian, 2015-12-17 04:19:56]

Description was changed from

==========
Add --perf-log flag to the dartanalyzer command

Also added a couple of performance tags to catch time spent in
the driver.

Sample usage:

$ dart bin/analyzer.dart lib/src/driver.dart lib/src/analyzer_impl.dart \
    --perf-log=perf.json
Analyzing [lib/src/driver.dart, lib/src/analyzer_impl.dart]...
No issues found
No issues found

$ cat perf.json
{
  "Driver._analyzeAll": 117,
  "WorkOrder.moveNext": 600,
  "io": 67,
  "AnalysisDriver.processOutputs": 445,
  "AnalyzerImpl._resolveLibrary": 6507,
  "resolve": 1111,
  "AnalyzerImpl.prepareErrors": 154,
  "other": 175,
  "total": 9176
}

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

to

==========
Add --x-perf-report flag to the dartanalyzer command
    
Also added a couple of performance tags to catch time spent in the driver.

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

[skybrian, 2015-12-17 04:20:27]

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
File pkg/analyzer_cli/lib/src/analyzer_impl.dart (right):

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/analyzer_impl.dart:127:
errorInfos.add(context.getErrors(source));
On 2015/12/16 15:08:43, Brian Wilkerson wrote:
> The original code looks weird to me. Seems like we should just have one line:
> 
> errorInfos.add(context.computeErrors(source));

Yes that does seem strange, but I think I'd like to defer the fix to another
patch.

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/analyzer_impl.dart:132: static final PerformanceTag
_prepareErrorsTag =
On 2015/12/16 15:08:43, Brian Wilkerson wrote:
> Don't forget to sort and format the file before committing it.

Formatted. It's not clear to me which order this file is sorted. What do you use
for this?

[skybrian, 2015-12-17 04:57:15]

Here's example output for the second patch:

{
  "platform": {
    "osType": "mac",
    "dartSdkVersion": "1.13.0 (Wed Nov 18 12:15:29 2015) on \"macos_x64\"",
    "checkedMode": true
  },
  "options": {
    "dartSdkPath": "/Users/skybrian/software/dart-sdk",
    "strongMode": false,
    "showPackageWarnings": false,
    "showSdkWarnings": false,
    "definedVariables": {},
    "packageRootPath": null,
    "packageConfigPath": null,
    "sourceFiles": [
      "lib/src/driver.dart",
      "lib/src/analyzer_impl.dart"
    ]
  },
  "totalElapsedTime": 18666,
  "performanceTags": {
    "Driver._analyzeAll": 214,
    "WorkOrder.moveNext": 1322,
    "io": 114,
    "AnalysisDriver.processOutputs": 1077,
    "AnalyzerImpl._resolveLibrary": 13523,
    "resolve": 1811,
    "AnalyzerImpl.prepareErrors": 281,
    "other": 324
  }
}

[Brian Wilkerson, 2015-12-17 15:05:42]

Still LGTM

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
File pkg/analyzer_cli/lib/src/analyzer_impl.dart (right):

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/analyzer_impl.dart:127:
errorInfos.add(context.getErrors(source));
sounds good

https://chromiumcodereview.appspot.com/1524413002/diff/1/pkg/analyzer_cli/lib...
pkg/analyzer_cli/lib/src/analyzer_impl.dart:132: static final PerformanceTag
_prepareErrorsTag =
The sort function available from either IntelliJ or Atom.

https://chromiumcodereview.appspot.com/1524413002/diff/40001/pkg/analyzer_cli...
File pkg/analyzer_cli/lib/src/perf_report.dart (right):

https://chromiumcodereview.appspot.com/1524413002/diff/40001/pkg/analyzer_cli...
pkg/analyzer_cli/lib/src/perf_report.dart:16: String makePerfReport(int
startTime, int endTime, CommandLineOptions options) {
We should add a version number to the output so that we can easily detect when
the format of the output has changed.

[skybrian, 2015-12-17 19:40:20]

Description was changed from

==========
Add --x-perf-report flag to the dartanalyzer command
    
Also added a couple of performance tags to catch time spent in the driver.

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

to

==========
Add --x-perf-report flag to the dartanalyzer command

Also added a couple of performance tags to catch time spent in the driver.

BUG=
R=brianwilkerson@google.com, paulberry@google.com

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

[skybrian, 2015-12-17 19:40:22]

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