Enforce single computeDelta() invocation.

Enforce single computeDelta() invocation.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[Siggi Cherem (dart-lang), 2017-08-10 22:43:30]

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

https://codereview.chromium.org/2993393002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:184: if
(_lastSignatures == null) {
could we also check here that _isComputeDeltaExecuting is false?

this was my main concern before: if its true, we might clear _lastSignature and
potentially produce a failure in computeDelta when trying to update
_lastSignatures in line 109

https://codereview.chromium.org/2993393002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2993393002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:235:
incrementalKernelGenerator.computeDelta();
we might want to store the future returned by this method and await for it at
the end of this test, so we don't have a lingering work happening after the test
is done.

https://codereview.chromium.org/2993393002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:239: await
incrementalKernelGenerator.computeDelta();
Could this be flaky?

In particular, is it at all possible for the first computeDelta to complete by
the time we reach this point? Or are we guaranteed that we have at least one
microtask pending (so it is not possible)? Right now it wouldn't be visible on
this test because if it completes early we would still throw because we haven't
`accepted` the delta, but if we address the comment below, this might be more
relevant.

https://codereview.chromium.org/2993393002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:241: } on StateError
{}
there are two reasons why computeDelta could throw a state error, let's check
that the error you are getting is the one about computeDelta is executing

[scheglov, 2017-08-10 23:46:50]

PTAL

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

https://codereview.chromium.org/2993393002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:184: if
(_lastSignatures == null) {
On 2017/08/10 22:43:29, Siggi Cherem (dart-lang) wrote:
> could we also check here that _isComputeDeltaExecuting is false?
> 
> this was my main concern before: if its true, we might clear _lastSignature
and
> potentially produce a failure in computeDelta when trying to update
> _lastSignatures in line 109

Done.

https://codereview.chromium.org/2993393002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2993393002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:235:
incrementalKernelGenerator.computeDelta();
On 2017/08/10 22:43:29, Siggi Cherem (dart-lang) wrote:
> we might want to store the future returned by this method and await for it at
> the end of this test, so we don't have a lingering work happening after the
test
> is done.

Done.

https://codereview.chromium.org/2993393002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:239: await
incrementalKernelGenerator.computeDelta();
On 2017/08/10 22:43:29, Siggi Cherem (dart-lang) wrote:
> Could this be flaky?
> 
> In particular, is it at all possible for the first computeDelta to complete by
> the time we reach this point? Or are we guaranteed that we have at least one
> microtask pending (so it is not possible)? Right now it wouldn't be visible on
> this test because if it completes early we would still throw because we
haven't
> `accepted` the delta, but if we address the comment below, this might be more
> relevant.

I changed computeDelta() to check _isComputeDeltaExecuting synchronously and set
it to false asynchronously, before returning the result.

https://codereview.chromium.org/2993393002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:241: } on StateError
{}
On 2017/08/10 22:43:29, Siggi Cherem (dart-lang) wrote:
> there are two reasons why computeDelta could throw a state error, let's check
> that the error you are getting is the one about computeDelta is executing

Well, there is no great way to do this.
I had to use contain(string).

[scheglov, 2017-08-11 00:37:29]

PTAL (I forgot to press Enter).

[Siggi Cherem (dart-lang), 2017-08-11 00:44:52]

lgtm, thanks!

https://codereview.chromium.org/2993393002/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2993393002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:24: static const
MSG_PENDING_COMPUTE =
not for this CL: even if we don't report them on the regular API, it might be
worth using messages.yaml as well. You can still extract the message and throw
it :) Or use `internalProblem` like we do in other parts of the compiler.

https://codereview.chromium.org/2993393002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:25: 'Another
computeDelta() invocation is still executing.';
Consider: Another => A

(since now this message is also shown when you call acceptLastDelta())

[scheglov, 2017-08-11 04:50:40]

https://codereview.chromium.org/2993393002/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2993393002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:24: static const
MSG_PENDING_COMPUTE =
On 2017/08/11 00:44:52, Siggi Cherem (dart-lang) wrote:
> not for this CL: even if we don't report them on the regular API, it might be
> worth using messages.yaml as well. You can still extract the message and throw
> it :) Or use `internalProblem` like we do in other parts of the compiler.

Yes, is seems like it might be useful to put into messages.yaml for consistency.
I will try to do this in the future.

https://codereview.chromium.org/2993393002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:25: 'Another
computeDelta() invocation is still executing.';
On 2017/08/11 00:44:52, Siggi Cherem (dart-lang) wrote:
> Consider: Another => A
> 
> (since now this message is also shown when you call acceptLastDelta())

Done.

[scheglov, 2017-08-11 04:51:13]

Description was changed from

==========
Enforce single computeDelta() invocation.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Enforce single computeDelta() invocation.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[scheglov, 2017-08-11 04:51:15]

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