Add "checked" parameter to Isolate.spawnUri.

Add "checked" parameter to Isolate.spawnUri.

R=iposva@google.com

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

[Lasse Reichstein Nielsen, 2015-05-29 12:18:03]

lrn@google.com changed reviewers:
+ sgjesse@google.com, zra@google.com

[Lasse Reichstein Nielsen, 2015-05-29 12:18:04]

One issue here is that the VM takes a command line flag as an override on all
isolates, not just as the checked-mode state of the initial isolate. That means
that a "checked:false" parameter is meaningless in checked mode.
I think the behavior should be changed so that the --checked argument on the
command line only affects the initial isolate, and all other isolates just
inherit that state by default.
That way there is no difference between running an isolate from another isolate
or from the command-line, and a unit-test framework can spawn isolates both
checked and unchecked, no matter how it was itself started.

[sra1, 2015-05-29 12:32:08]

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

[sra1, 2015-05-29 12:32:08]

How do you propose to support this for dart2js?

[Lasse Reichstein Nielsen, 2015-05-29 12:37:47]

On 2015/05/29 12:32:08, sra1 wrote:
> How do you propose to support this for dart2js?

By ignoring it completely.
As the documentation says, if the spawned code is pre-compiled, then it might
not be possible to change its checked-mode state. That is exactly the case for
dart2js. Possibly also for a VM snapshot.

If dart2js decides to detect entry-points that are recognizable (like
"Isolate.spawnUri(Uri.parse("literal URL"), ...)") then it can possibly also
infer the flags from there and compile the script directly.

But it means that the test will fail on dart2js, so at least I need to fix the
status file.
I consider it also failing on the VM, but I fixed that by not running the test
in checked mode (yet).

[Ivan Posva, 2015-05-29 15:18:16]

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

[Ivan Posva, 2015-05-29 15:18:18]

-Ivan

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate.cc#...
runtime/lib/isolate.cc:185:
child_isolate->set_strict_compilation(parent_isolate->strict_compilation());
This should be handled when we create the spawn state. Basically in
spawnFunction we inherit from the parent isolate and in spawnUri we should
handle the tri-state parameter.

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate_pat...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate_pat...
runtime/lib/isolate_patch.dart:316: assert((checked = true));
?

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate_pat...
runtime/lib/isolate_patch.dart:319: paused, checked, packageRootString);
Please pass the exact parameter down to the native code, so that we can inherit
when setting up the isolate spawn state.

https://codereview.chromium.org/1154673004/diff/20001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1154673004/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:386: void set_strict_compilation(bool value) {
strict_compilation_ = value; }
I know this is not from your change. "strict compilation" is not a concept the
VM really knows. What we are interested in here is are assertions enabled and
are type checks enabled. We should split these two up similarly to the two flags
that can be given to the VM.

If no override is given when spawning an isolate these two values should be
inherited independently.

https://codereview.chromium.org/1154673004/diff/20001/sdk/lib/isolate/isolate...
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/1154673004/diff/20001/sdk/lib/isolate/isolate...
sdk/lib/isolate/isolate.dart:202: * and `false` otherwise.
You could also say that it inherits the checked mode state from the current
isolate, although I am not 100% sure which way is more understandable.

https://codereview.chromium.org/1154673004/diff/20001/sdk/lib/isolate/isolate...
sdk/lib/isolate/isolate.dart:218: Uri packageRoot});
The order in the function signature and in the documentation do not match.

[Ivan Posva, 2015-05-29 15:23:46]

On 2015/05/29 12:37:47, Lasse Reichstein Nielsen wrote:
> On 2015/05/29 12:32:08, sra1 wrote:
> > How do you propose to support this for dart2js?
> 
> By ignoring it completely.
> As the documentation says, if the spawned code is pre-compiled, then it might
> not be possible to change its checked-mode state. That is exactly the case for
> dart2js. Possibly also for a VM snapshot.

Snapshots are checked mode agnostic. The VM will enforce assertions and type
checks correctly regardless whether a program was launched from a snapshot or
from sources. Otherwise we would not be able to run with checked mode for all of
the core libraries as they are loaded from a snapshot stored in the dart binary.

-Ivan

> If dart2js decides to detect entry-points that are recognizable (like
> "Isolate.spawnUri(Uri.parse("literal URL"), ...)") then it can possibly also
> infer the flags from there and compile the script directly.
> 
> But it means that the test will fail on dart2js, so at least I need to fix the
> status file.
> I consider it also failing on the VM, but I fixed that by not running the test
> in checked mode (yet).

[Lasse Reichstein Nielsen, 2015-06-02 15:26:44]

PTAL

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate.cc#...
runtime/lib/isolate.cc:185:
child_isolate->set_strict_compilation(parent_isolate->strict_compilation());
On 2015/05/29 15:18:17, Ivan Posva wrote:
> This should be handled when we create the spawn state. Basically in
> spawnFunction we inherit from the parent isolate and in spawnUri we should
> handle the tri-state parameter.

Done.

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate_pat...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate_pat...
runtime/lib/isolate_patch.dart:316: assert((checked = true));
Just a way to get the checked-mode state of the current isolate in Dart code.
If the VM allows assertion statements to be different from type assertions, then
it's probably not safe.

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate_pat...
runtime/lib/isolate_patch.dart:319: paused, checked, packageRootString);
Done.

https://codereview.chromium.org/1154673004/diff/20001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1154673004/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:386: void set_strict_compilation(bool value) {
strict_compilation_ = value; }
I'll leave that for a later CL.

https://codereview.chromium.org/1154673004/diff/20001/sdk/lib/isolate/isolate...
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/1154673004/diff/20001/sdk/lib/isolate/isolate...
sdk/lib/isolate/isolate.dart:202: * and `false` otherwise.
On 2015/05/29 15:18:17, Ivan Posva wrote:
> You could also say that it inherits the checked mode state from the current
> isolate, although I am not 100% sure which way is more understandable.

Done.

https://codereview.chromium.org/1154673004/diff/20001/sdk/lib/isolate/isolate...
sdk/lib/isolate/isolate.dart:218: Uri packageRoot});
On 2015/05/29 15:18:17, Ivan Posva wrote:
> The order in the function signature and in the documentation do not match.

Done.

[kevmoo, 2015-06-02 17:42:54]

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

[kevmoo, 2015-06-02 17:42:55]

DBC

https://codereview.chromium.org/1154673004/diff/40001/sdk/lib/isolate/isolate...
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/1154673004/diff/40001/sdk/lib/isolate/isolate...
sdk/lib/isolate/isolate.dart:191: * WARNING: The [checked] parameter is not
implemented on all platforms yet.
We have no plan to implement this in dart2js, right?

We should be explicit about which platform it is implemented on and the behavior
on unimplemented platforms – noop?

https://codereview.chromium.org/1154673004/diff/40001/tests/isolate/checked_t...
File tests/isolate/checked_test.dart (right):

https://codereview.chromium.org/1154673004/diff/40001/tests/isolate/checked_t...
tests/isolate/checked_test.dart:10: void main([args, message]) {
How will this be marked in the status file for dart2js? Expect a failure?

https://codereview.chromium.org/1154673004/diff/40001/tests/isolate/checked_t...
tests/isolate/checked_test.dart:15: if (isChecked) return;  // Skip this test in
checked mode.
should this 'check' happen in the status file?

[Ivan Posva, 2015-06-02 17:49:32]

LGTM with request to add a TODO...

Thanks,
-Ivan

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate_pat...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/1154673004/diff/20001/runtime/lib/isolate_pat...
runtime/lib/isolate_patch.dart:316: assert((checked = true));
On 2015/06/02 15:26:44, Lasse Reichstein Nielsen wrote:
> Just a way to get the checked-mode state of the current isolate in Dart code.
> If the VM allows assertion statements to be different from type assertions,
then
> it's probably not safe.

Assertions and checked mode are two different concepts in the VM which are both
enabled by passing the --checked flag on the command line. They can be enabled
individually if so desired.

https://codereview.chromium.org/1154673004/diff/40001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/1154673004/diff/40001/runtime/lib/isolate.cc#...
runtime/lib/isolate.cc:184:
child_isolate->set_strict_compilation(state->checked_mode());
Please add:
// TODO(iposva): Evaluate whether it is OK to override the embedder's setup.

https://codereview.chromium.org/1154673004/diff/40001/runtime/lib/isolate_pat...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/1154673004/diff/40001/runtime/lib/isolate_pat...
runtime/lib/isolate_patch.dart:306: { bool paused: false, bool checked, Uri
packageRoot }) {
I assume we will add the package spec file Uri in an upcoming change?

[Lasse Reichstein Nielsen, 2015-06-03 06:55:25]

https://codereview.chromium.org/1154673004/diff/40001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/1154673004/diff/40001/runtime/lib/isolate.cc#...
runtime/lib/isolate.cc:184:
child_isolate->set_strict_compilation(state->checked_mode());
On 2015/06/02 17:49:31, Ivan Posva wrote:
> Please add:
> // TODO(iposva): Evaluate whether it is OK to override the embedder's setup.

Done.

https://codereview.chromium.org/1154673004/diff/40001/runtime/lib/isolate_pat...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/1154673004/diff/40001/runtime/lib/isolate_pat...
runtime/lib/isolate_patch.dart:306: { bool paused: false, bool checked, Uri
packageRoot }) {
As do I.

https://codereview.chromium.org/1154673004/diff/40001/sdk/lib/isolate/isolate...
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/1154673004/diff/40001/sdk/lib/isolate/isolate...
sdk/lib/isolate/isolate.dart:191: * WARNING: The [checked] parameter is not
implemented on all platforms yet.
On 2015/06/02 17:42:55, kevmoo wrote:
> We have no plan to implement this in dart2js, right?

We could implement it by appending "?checked" to the URI when fetching the file.
Then the server could potentially send a different file if it has one (or
compile it while we wait, if not in a rush).
Dart2js can decide what to do, but doing nothing is an option.

> We should be explicit about which platform it is implemented on and the
behavior
> on unimplemented platforms – noop?

I prefer not to mention specific implementations in the system libraries. It
doesn't make sense in the long run where there might be dozens of platforms.
Should we mention Sky? Dartium? 
I'd prefer to leave it as this - a platform implements this if it can, otherwise
it may be ignored. I'll add some more text to elaborate that.

https://codereview.chromium.org/1154673004/diff/40001/tests/isolate/checked_t...
File tests/isolate/checked_test.dart (right):

https://codereview.chromium.org/1154673004/diff/40001/tests/isolate/checked_t...
tests/isolate/checked_test.dart:10: void main([args, message]) {
Yes, thanks for reminding me :)

https://codereview.chromium.org/1154673004/diff/40001/tests/isolate/checked_t...
tests/isolate/checked_test.dart:15: if (isChecked) return;  // Skip this test in
checked mode.
I'd prefer to not have it at all, but that requires the VM to change its
behavior so the command line flag doesn't override the isolate parameter.
Until we agree to doing that, the test is neutered.

[Lasse Reichstein Nielsen, 2015-06-03 10:33:06]

Committed patchset #5 (id:80001) manually as
5a843ebbf31c16abf92e9afbd8bf78db5deb9b26 (presubmit successful).