Precompute `IgnoreInfo` in Scanner.

Precompute `IgnoreInfo` in Scanner.

The existing support for `ignore` filtering requires the error generator task to depend on parsed sources as input.  As it happens, these parse results get flushed between the parse task and error generation, meaning that they need to be recomputed for EVERY source.  This change moves ignore detection into the scanner which now produces a new result (akin to LineInfo) that can be used at error generation time (and will not be flushed).

Local profiling shows this change making a roughly 10% improvement to overall analysis time for `flutter analyze`.  Server-based analysis should enjoy a similar benefit.

A few thoughts for further refinement:

* can we NOT produce ignore results for sources whose errors we will not generate?
* can we (and should we) improve the regexp-based approach?

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

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

[pquitslund, 2016-05-27 17:19:26]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[pquitslund, 2016-05-27 17:57:40]

Regarding unnecessary calculation of ignores, assuming we're not generating
errors for SDK resources can we check source.isInSystemLibrary?

[scheglov, 2016-05-27 18:05:47]

lgtm

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/source.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:182: class IgnoreInfo {
This could live in src/dart.dart

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:200: bool get hasIgnores =>
ignores.length > 0;
=> ignores.isNotEmpty;

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:5385: <ResultDescriptor>[LINE_INFO,
IGNORE_INFO, SCAN_ERRORS, TOKEN_STREAM],
We try to keep these sorted.
IGNORE_INFO would be the first item in the list.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:5397: new RegExp(r'(//[ ]*ignore:)(.*)$',
multiLine: true);
Do we use the (//ignore:) group?
I don't know whether this might have a performance cost.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/task/gener...
File pkg/analyzer/lib/task/general.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/task/gener...
pkg/analyzer/lib/task/general.dart:20: new
ResultDescriptor<IgnoreInfo>('IGNORE_INFO', null);
Do we need this as a public information?

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/dart_test.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/dart_test.dart:4486: expect(outputs[IGNORE_INFO],
isNotNull);
Would be nice to test the computed IgnoreInfo here.

[Brian Wilkerson, 2016-05-27 18:21:18]

lgtm

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:5503: static IgnoreInfo
calculateIgnores(String content, LineInfo info) {
This could be a static method on IgnoreInfo.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:5511:
ignoreInfo.add(info.getLocation(match.start).lineNumber, code.trim());
It would be more efficient to define LineInfo.addAll and use it.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:5514: return ignoreInfo;
For memory efficiency, it would be good to use a single empty instance of
IgnoreInfo when there are no matches.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/task/gener...
File pkg/analyzer/lib/task/general.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/task/gener...
pkg/analyzer/lib/task/general.dart:20: new
ResultDescriptor<IgnoreInfo>('IGNORE_INFO', null);
I'd make it an internal result until we find a reason to make it public.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/dart_test.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/dart_test.dart:4486: expect(outputs[IGNORE_INFO],
isNotNull);
If the method in the task gets moved to the class, then we could test this by
adding tests for IngoreInfo.

[pquitslund, 2016-05-27 21:50:53]

Thanks!

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/source.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:182: class IgnoreInfo {
On 2016/05/27 18:05:47, scheglov wrote:
> This could live in src/dart.dart

Done.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/source.dart:200: bool get hasIgnores =>
ignores.length > 0;
On 2016/05/27 18:05:47, scheglov wrote:
> => ignores.isNotEmpty;

Done.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:5385: <ResultDescriptor>[LINE_INFO,
IGNORE_INFO, SCAN_ERRORS, TOKEN_STREAM],
On 2016/05/27 18:05:47, scheglov wrote:
> We try to keep these sorted.
> IGNORE_INFO would be the first item in the list.

Done.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:5397: new RegExp(r'(//[ ]*ignore:)(.*)$',
multiLine: true);
On 2016/05/27 18:05:47, scheglov wrote:
> Do we use the (//ignore:) group?
> I don't know whether this might have a performance cost.

Ah!  Yea.  Great catch.  No need for that first group.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:5503: static IgnoreInfo
calculateIgnores(String content, LineInfo info) {
On 2016/05/27 18:21:18, Brian Wilkerson wrote:
> This could be a static method on IgnoreInfo.

Done.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:5511:
ignoreInfo.add(info.getLocation(match.start).lineNumber, code.trim());
On 2016/05/27 18:21:18, Brian Wilkerson wrote:
> It would be more efficient to define LineInfo.addAll and use it.

Done.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:5514: return ignoreInfo;
On 2016/05/27 18:21:18, Brian Wilkerson wrote:
> For memory efficiency, it would be good to use a single empty instance of
> IgnoreInfo when there are no matches.

Good idea.  Done

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/task/gener...
File pkg/analyzer/lib/task/general.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/lib/task/gener...
pkg/analyzer/lib/task/general.dart:20: new
ResultDescriptor<IgnoreInfo>('IGNORE_INFO', null);
On 2016/05/27 18:21:18, Brian Wilkerson wrote:
> I'd make it an internal result until we find a reason to make it public.

Done.

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/dart_test.dart (right):

https://codereview.chromium.org/2011183004/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/dart_test.dart:4486: expect(outputs[IGNORE_INFO],
isNotNull);
On 2016/05/27 18:21:18, Brian Wilkerson wrote:
> If the method in the task gets moved to the class, then we could test this by
> adding tests for IngoreInfo.

Done.

[pquitslund, 2016-05-27 21:52:24]

Description was changed from

==========
Precompute `IgnoreInfo` in Scanner.

The existing support for `ignore` filtering requires the error generator task to
depend on parsed sources as input.  As it happens, these parse results get
flushed between the parse task and error generation, meaning that they need to
be recomputed for EVERY source.  This change moves ignore detection into the
scanner which now produces a new result (akin to LineInfo) that can be used at
error generation time (and will not be flushed).

Local profiling shows this change making a roughly 10% improvement to overall
analysis time for `flutter analyze`.  Server-based analysis should enjoy a
similar benefit.

A few thoughts for further refinement:

* can we NOT produce ignore results for sources whose errors we will not
generate?
* can we (and should we) improve the regexp-based approach?

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

to

==========
Precompute `IgnoreInfo` in Scanner.

The existing support for `ignore` filtering requires the error generator task to
depend on parsed sources as input.  As it happens, these parse results get
flushed between the parse task and error generation, meaning that they need to
be recomputed for EVERY source.  This change moves ignore detection into the
scanner which now produces a new result (akin to LineInfo) that can be used at
error generation time (and will not be flushed).

Local profiling shows this change making a roughly 10% improvement to overall
analysis time for `flutter analyze`.  Server-based analysis should enjoy a
similar benefit.

A few thoughts for further refinement:

* can we NOT produce ignore results for sources whose errors we will not
generate?
* can we (and should we) improve the regexp-based approach?

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

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

[pquitslund, 2016-05-27 21:52:25]

Committed patchset #2 (id:20001) manually as
c77781d27e54fdf04721b8b5890576a91619e976 (presubmit successful).