Implement spawnFunction from the new isolate api.

Implement spawnFunction from the new isolate api.

Still need to implement spawnUri, which will be the more difficult one.
Committed: https://code.google.com/p/dart/source/detail?r=5441

[Siggi Cherem (dart-lang), 2012-03-12 21:28:49]

very exciting!

https://chromiumcodereview.appspot.com/9691005/diff/3001/tests/isolate/isolat...
File tests/isolate/isolate.status (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/tests/isolate/isolat...
tests/isolate/isolate.status:13: # *APIv2*: Skip # new api for isolates not
implemented in the VM yet.
you might want to continue skiping the spawnUri tests (or mark them as failing)

[turnidge, 2012-03-12 22:38:47]

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:512: // TODO(turnidge): Throw a different exception type
here.
Siggi -- what type of exception do we want to throw here?

https://chromiumcodereview.appspot.com/9691005/diff/3001/tests/isolate/isolat...
File tests/isolate/isolate.status (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/tests/isolate/isolat...
tests/isolate/isolate.status:13: # *APIv2*: Skip # new api for isolates not
implemented in the VM yet.
On 2012/03/12 21:28:49, sigmund wrote:
> you might want to continue skiping the spawnUri tests (or mark them as
failing)

Oops, forgot to uncomment that.

[Siggi Cherem (dart-lang), 2012-03-12 22:57:44]

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:503: if (ctx.num_variables() > 0) {
is this check redundant with the check above for is_static?

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:512: // TODO(turnidge): Throw a different exception type
here.
On 2012/03/12 22:38:48, turnidge wrote:
> Siggi -- what type of exception do we want to throw here?

How about we introduce 'IsolateSpawnException' (making this part of
dart:isolate)? Other ideas?

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:532: FATAL1("Failed to start isolate thread: error %d",
result);
should this be fatal or also throw the IsolateSpawnException?

[siva, 2012-03-12 23:25:41]

LGTM.

We should also consider adding a VM unit test for the new spawn functionality in
isolate_test.cc

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:404: const Library& lib = Library::Handle(cls.library());
Assert cls represents implicit top level in 'lib'

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:442: state->set_isolate(child_isolate);
We are assuming that the callback will load up the right library, It might be a
good idea to do a lookup of the library and function to ascertain that they
exist in the child isolate before declaring that the isolate was successfully
created.

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:477:
Function::Handle(lib.LookupLocalFunction(func_name));
ASSERT(!func.IsNull());

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:495: const Closure& closure =
Closure::CheckedHandle(arguments->At(0));
This should be
GET_NATIVE_ARGUMENT(Closure, closure, arguments->At(0));

so that an illegal argument exception is thrown when a non closure object is
passed in instead of calling FATAL.

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:502: }
Don't we also have to check that it is a top level static function?

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:532: FATAL1("Failed to start isolate thread: error %d",
result);
Why does this have to be a Fatal and not an exception?

https://chromiumcodereview.appspot.com/9691005/diff/3001/tests/isolate/isolat...
File tests/isolate/isolate.status (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/tests/isolate/isolat...
tests/isolate/isolate.status:9: *BrowserNegativeTest: Skip
BrowserTest and BrowserNegativeTest were not being skipped before why are they
being skipped now?

[Anton Muhin, 2012-03-13 15:52:10]

DBQ

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
File runtime/lib/isolate.dart (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.dart:170: _spawnFunction(void topLevelFunction()) native
"isolate_spawnFunction";
just curious.  apparently something like _spawnFunction(() => main()) won't
work, correct?

[turnidge, 2012-03-13 18:13:48]

Siva,

I added a bunch of new files to the CL, so it is probably worth taking another
pass.

In particular, I've added a String::NewFormat function because I got tired of
having bad error messages in exceptions.

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:404: const Library& lib = Library::Handle(cls.library());
On 2012/03/12 23:25:41, asiva wrote:
> Assert cls represents implicit top level in 'lib'

Added an assert that cls.IsTopLevel().

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:442: state->set_isolate(child_isolate);
On 2012/03/12 23:25:41, asiva wrote:
> We are assuming that the callback will load up the right library, It might be
a
> good idea to do a lookup of the library and function to ascertain that they
> exist in the child isolate before declaring that the isolate was successfully
> created.

It seems to me that if this occurs, it is an internal error, right?  Is there
any wa that this will occur under normal circumstances?  I have added an
assertion that the function always resolves in RunIsolate2.  Is that sufficient?

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:477:
Function::Handle(lib.LookupLocalFunction(func_name));
On 2012/03/12 23:25:41, asiva wrote:
> ASSERT(!func.IsNull());

Done.

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:495: const Closure& closure =
Closure::CheckedHandle(arguments->At(0));
On 2012/03/12 23:25:41, asiva wrote:
> This should be
> GET_NATIVE_ARGUMENT(Closure, closure, arguments->At(0));
> 
> so that an illegal argument exception is thrown when a non closure object is
> passed in instead of calling FATAL.

Done.

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:502: }
On 2012/03/12 23:25:41, asiva wrote:
> Don't we also have to check that it is a top level static function?

Done.

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:503: if (ctx.num_variables() > 0) {
On 2012/03/12 22:57:44, sigmund wrote:
> is this check redundant with the check above for is_static?

I believe it is.  Made it an ASSERT.

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:512: // TODO(turnidge): Throw a different exception type
here.
On 2012/03/12 22:57:44, sigmund wrote:
> On 2012/03/12 22:38:48, turnidge wrote:
> > Siggi -- what type of exception do we want to throw here?
> 
> How about we introduce 'IsolateSpawnException' (making this part of
> dart:isolate)? Other ideas?

I've added IsolateSpawnException as part of this change.

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:532: FATAL1("Failed to start isolate thread: error %d",
result);
On 2012/03/12 23:25:41, asiva wrote:
> Why does this have to be a Fatal and not an exception?

I was just cribbing from the existing code.  Changed it to an exception.

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
File runtime/lib/isolate.dart (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.dart:170: _spawnFunction(void topLevelFunction()) native
"isolate_spawnFunction";
On 2012/03/13 15:52:10, antonmuhin wrote:
> just curious.  apparently something like _spawnFunction(() => main()) won't
> work, correct?

We currently disallow it, yes.  We could permit it in the future though.

https://chromiumcodereview.appspot.com/9691005/diff/3001/tests/isolate/isolat...
File tests/isolate/isolate.status (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/tests/isolate/isolat...
tests/isolate/isolate.status:9: *BrowserNegativeTest: Skip
They were included as part of the *APIv2* below... 

On 2012/03/12 23:25:41, asiva wrote:
> BrowserTest and BrowserNegativeTest were not being skipped before why are they
> being skipped now?

[siva, 2012-03-13 19:45:21]

LGTM

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9691005/diff/3001/runtime/lib/isolate....
runtime/lib/isolate.cc:442: state->set_isolate(child_isolate);
Since the callback is something that the embedder registers we are executing
code outside the VM (external code) and that is why I feel we should treat it as
an error and not an internal error. Kind of similar to the checks we have when
somebody invokes a dart API method.


On 2012/03/13 18:13:48, turnidge wrote:
> On 2012/03/12 23:25:41, asiva wrote:
> > We are assuming that the callback will load up the right library, It might
be
> a
> > good idea to do a lookup of the library and function to ascertain that they
> > exist in the child isolate before declaring that the isolate was
successfully
> > created.
> 
> It seems to me that if this occurs, it is an internal error, right?  Is there
> any wa that this will occur under normal circumstances?  I have added an
> assertion that the function always resolves in RunIsolate2.  Is that
sufficient?

https://chromiumcodereview.appspot.com/9691005/diff/4005/lib/isolate/isolate_...
File lib/isolate/isolate_api.dart (right):

https://chromiumcodereview.appspot.com/9691005/diff/4005/lib/isolate/isolate_...
lib/isolate/isolate_api.dart:9: }
By defining it here are you making IsolateSpawnException a first class type and
part of the Isolate API?

I don't know what the consensus is on making Exception types part of the library
API...

https://chromiumcodereview.appspot.com/9691005/diff/4005/runtime/vm/object_te...
File runtime/vm/object_test.cc (right):

https://chromiumcodereview.appspot.com/9691005/diff/4005/runtime/vm/object_te...
runtime/vm/object_test.cc:606: TEST_CASE(StringFormat) {
const char* hello_str = "Hello World!";
int32_t hello_len = strlen(hello_str);

https://chromiumcodereview.appspot.com/9691005/diff/4005/runtime/vm/object_te...
runtime/vm/object_test.cc:627: EXPECT_EQ('!', str.CharAt(11));
EXPECT_EQ(str.Equals(hello_str));

instead of these individual checks?

[turnidge, 2012-03-13 21:23:37]

https://chromiumcodereview.appspot.com/9691005/diff/4005/lib/isolate/isolate_...
File lib/isolate/isolate_api.dart (right):

https://chromiumcodereview.appspot.com/9691005/diff/4005/lib/isolate/isolate_...
lib/isolate/isolate_api.dart:9: }
On 2012/03/13 19:45:21, asiva wrote:
> By defining it here are you making IsolateSpawnException a first class type
and
> part of the Isolate API?
> 
> I don't know what the consensus is on making Exception types part of the
library
> API...

No idea what the consensus is.  Is it okay to add it for now and hide it later?

https://chromiumcodereview.appspot.com/9691005/diff/4005/runtime/vm/object_te...
File runtime/vm/object_test.cc (right):

https://chromiumcodereview.appspot.com/9691005/diff/4005/runtime/vm/object_te...
runtime/vm/object_test.cc:606: TEST_CASE(StringFormat) {
On 2012/03/13 19:45:21, asiva wrote:
> const char* hello_str = "Hello World!";
> int32_t hello_len = strlen(hello_str);

Done.

https://chromiumcodereview.appspot.com/9691005/diff/4005/runtime/vm/object_te...
runtime/vm/object_test.cc:627: EXPECT_EQ('!', str.CharAt(11));
On 2012/03/13 19:45:21, asiva wrote:
> EXPECT_EQ(str.Equals(hello_str));
> 
> instead of these individual checks?

Just mimicking existing tests.  Didn't realize we had an Equals that took a
char*.  Fixed.

[Siggi Cherem (dart-lang), 2012-03-13 23:00:13]

lgtm

https://chromiumcodereview.appspot.com/9691005/diff/5017/lib/isolate/isolate_...
File lib/isolate/isolate_api.dart (right):

https://chromiumcodereview.appspot.com/9691005/diff/5017/lib/isolate/isolate_...
lib/isolate/isolate_api.dart:5: class IsolateSpawnException implements Exception
{
(nit): move this to the bottom, so that the top of this file contains the common
APIs that people will use. 
also + a 1-line comment so that it gets documented for api.dartlang.com