IncrementalKernalGenerator watch source callback

IncrementalKernalGenerator watch source callback

Update the IncrementalKernalGenerator.computeDelta() method
with an optional "watch this source" callback.
This new callback notifies the caller when a source should be watched
for modifications, and when a source is no longer part of the program
and thus no longer needs to be watched.

R=paulberry@google.com

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

[danrubel, 2017-01-25 20:05:12]

danrubel@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2017-01-25 22:03:34]

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/increment...
File pkg/front_end/lib/incremental_kernel_generator.dart (right):

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:72: /// used when compiling
the program. If during a subsequent call to
Please add a sentence explaining that the file contents won't be read until the
future returned by [watch] completes.  (This is necessary to avoid race
conditions, because we need to make sure the client has actually started
watching the file before we read it).

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:76: ///
Please add a sentence explaining that multiple invocations of [watch] may be
running concurrently.  (This prevents [watch] from becoming a concurrency
bottleneck).

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:80: //
TODO(danrubel) rework watch invocation to eliminate race condition
Please add my name to this TODO as well.

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:82: if (watch !=
null) watch(uri, true);
Please change to:

if (watch != null) await watch(uri, true);

It doesn't address the race condition, but it at least ensures that all calls to
[watch] have completed by the time [computeDelta] returns (which I think would
be reasonably expected by client code).

[danrubel, 2017-01-25 22:40:41]

Comments addressed. PTAL.

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/increment...
File pkg/front_end/lib/incremental_kernel_generator.dart (right):

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:72: /// used when compiling
the program. If during a subsequent call to
On 2017/01/25 22:03:33, Paul Berry wrote:
> Please add a sentence explaining that the file contents won't be read until
the
> future returned by [watch] completes.  (This is necessary to avoid race
> conditions, because we need to make sure the client has actually started
> watching the file before we read it).

Done.

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:76: ///
On 2017/01/25 22:03:34, Paul Berry wrote:
> Please add a sentence explaining that multiple invocations of [watch] may be
> running concurrently.  (This prevents [watch] from becoming a concurrency
> bottleneck).

Done.

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:80: //
TODO(danrubel) rework watch invocation to eliminate race condition
On 2017/01/25 22:03:34, Paul Berry wrote:
> Please add my name to this TODO as well.

I replaced my name with yours both here and below as you will probably be
addressing these TODOs. PLMK if that is not correct and I'll change them to
include both our names.

https://codereview.chromium.org/2653143003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:82: if (watch !=
null) watch(uri, true);
On 2017/01/25 22:03:34, Paul Berry wrote:
> Please change to:
> 
> if (watch != null) await watch(uri, true);
> 
> It doesn't address the race condition, but it at least ensures that all calls
to
> [watch] have completed by the time [computeDelta] returns (which I think would
> be reasonably expected by client code).

That seems reasonable. I had to restructure the loop somewhat to get await to
work as expected. There's probably a better way of doing this so that watch does
not become a bottleneck, but I'll leave that for when it's reworked to eliminate
the watch race condition.

[Paul Berry, 2017-01-25 23:33:43]

lgtm, thanks!

[danrubel, 2017-01-26 13:25:36]

Description was changed from

==========
IncrementalKernalGenerator watch source callback

Update the IncrementalKernalGenerator.computeDelta() method
with an optional "watch this source" callback.
This new callback notifies the caller when a source should be watched
for modifications, and when a source is no longer part of the program
and thus no longer needs to be watched.
==========

to

==========
IncrementalKernalGenerator watch source callback

Update the IncrementalKernalGenerator.computeDelta() method
with an optional "watch this source" callback.
This new callback notifies the caller when a source should be watched
for modifications, and when a source is no longer part of the program
and thus no longer needs to be watched.

R=paulberry@google.com

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

[danrubel, 2017-01-26 13:25:37]

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