Convert most of the javascript isolate code into Dart, inject JS code only when

Convert most of the javascript isolate code into Dart, inject JS code only when
isolates are used by the application.

This brings 'hello world' down to 1.2k

Committed: https://code.google.com/p/dart/source/detail?r=1581

[Siggi Cherem (dart-lang), 2011-11-16 17:36:26]

there is still lots of clean up that we can do, but I thought this was a good
chunk to submit first.

Most of the code was a direct translation from the JS code, just some additional
type information :)

The code handling workers especially needs more work.

[jimhug, 2011-11-16 18:00:45]

LGTM!

Do you have a number for how the size of the trivial isolate sample/isolate
version of hello world changes before and after this CL?  I'd hope/expect that
it would be roughly unchanged.

http://codereview.chromium.org/8577003/diff/3010/frog/frogsh
File frog/frogsh (right):

http://codereview.chromium.org/8577003/diff/3010/frog/frogsh#newcode22690
frog/frogsh:22690: main();
Yay! 460 fewer lines! And this simple main() looks simpler too.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode35
frog/lib/isolate.dart:35: _globalState.currentIsolate = entryIsolate;
I'm glad there's a bug here - this should be fixed.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.js
File frog/lib/isolate.js (left):

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.js#oldcode1
frog/lib/isolate.js:1: // Copyright (c) 2011, the Dart project authors.  Please
see the AUTHORS file
Yay!

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart
File frog/value.dart (right):

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart#newcode300
frog/value.dart:300: * callbacks are executed withing the context of the isolate
that created them
Nit: within

[Siggi Cherem (dart-lang), 2011-11-16 18:15:35]

On 2011/11/16 18:00:45, jimhug wrote:
> LGTM!
> 
> Do you have a number for how the size of the trivial isolate sample/isolate
> version of hello world changes before and after this CL?  I'd hope/expect that
> it would be roughly unchanged.
> 

It is similar, but there was a small increase. It was 194K before, 204K after.
The extra 10K seems to mainly because there is more structure in the code (I
added a few classes and methods) and we use a dart datastructure that wasn't
used before (queue).

> http://codereview.chromium.org/8577003/diff/3010/frog/frogsh
> File frog/frogsh (right):
> 
> http://codereview.chromium.org/8577003/diff/3010/frog/frogsh#newcode22690
> frog/frogsh:22690: main();
> Yay! 460 fewer lines! And this simple main() looks simpler too.
> 
> http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart
> File frog/lib/isolate.dart (right):
> 
>
http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode35
> frog/lib/isolate.dart:35: _globalState.currentIsolate = entryIsolate;
> I'm glad there's a bug here - this should be fixed.
> 
> http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.js
> File frog/lib/isolate.js (left):
> 
> http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.js#oldcode1
> frog/lib/isolate.js:1: // Copyright (c) 2011, the Dart project authors. 
Please
> see the AUTHORS file
> Yay!
> 
> http://codereview.chromium.org/8577003/diff/3010/frog/value.dart
> File frog/value.dart (right):
> 
> http://codereview.chromium.org/8577003/diff/3010/frog/value.dart#newcode300
> frog/value.dart:300: * callbacks are executed withing the context of the
isolate
> that created them
> Nit: within

[mattsh, 2011-11-16 19:09:17]

Very nice.  Some comments below.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode21
frog/lib/isolate.dart:21: * (e.g. hello world), this call is not emited.
emitted

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode23
frog/lib/isolate.dart:23: void startAsIsolate(entry) {
startRootIsolate

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode28
frog/lib/isolate.dart:28: final entryIsolate = new IsolateContext();
maybe rootContext

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode41
frog/lib/isolate.dart:41: /** Global state associated with the current worker.
See [_globalState]. */
mabye call this WorkerState or ThreadState?  It looks like there is one instance
of this per-worker plus one instance for the UI thread, is that right?
Actually reading more, it looks like this merges together fields that are global
to all workers (like 'supportsWorkers') and fields that are per-worker (like
'topEventLoop', and also fields that are only needed by the main thread (like
the 'workers' registy).  Perhaps have separate structures for each of these? 
(e.g. GlobalState,   ThreadState, and MainThreadState)

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode57
frog/lib/isolate.dart:57: IsolateContext currentIsolate = null;
isolateContext

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode60
frog/lib/isolate.dart:60: IsolateContext rootIsolate = null;
rootContext

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode66
frog/lib/isolate.dart:66: bool inWorker;
inWorker suggests this value might change.  Perhaps calls it 'isWorker'?

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode95
frog/lib/isolate.dart:95: MainWorker mainWorker;
I think we're using somewhat confusing terminology, we should decide if we want
to call the main window thread to be considered a "worker" or not.  (Elsewhere
we use the term 'worker' to mean everything except the main thread, so we should
decide what it means.)

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode98
frog/lib/isolate.dart:98: Map<int, var> workers;
I think let's have a separate structure for fields only needed in the main
thread.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcod...
frog/lib/isolate.dart:249: static Function _platformDefer() native """
platformDefer is kind of vague, suggest rename to _setTimeout (which can be null
if the underlying support is not available).

[Siggi Cherem (dart-lang), 2011-11-17 02:03:04]

thanks for the comments. I'm addressing many of them in a follow up CL.

I'm still not sure on the best way to cleanup the worker/thread/global names,
but we should definitively do that.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode21
frog/lib/isolate.dart:21: * (e.g. hello world), this call is not emited.
On 2011/11/16 19:09:17, mattsh wrote:
> emitted

Done.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode23
frog/lib/isolate.dart:23: void startAsIsolate(entry) {
On 2011/11/16 19:09:17, mattsh wrote:
> startRootIsolate

Done.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode28
frog/lib/isolate.dart:28: final entryIsolate = new IsolateContext();
On 2011/11/16 19:09:17, mattsh wrote:
> maybe rootContext

Done.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode41
frog/lib/isolate.dart:41: /** Global state associated with the current worker.
See [_globalState]. */
On 2011/11/16 19:09:17, mattsh wrote:
> mabye call this WorkerState or ThreadState?  It looks like there is one
instance
> of this per-worker plus one instance for the UI thread, is that right?
> Actually reading more, it looks like this merges together fields that are
global
> to all workers (like 'supportsWorkers') and fields that are per-worker (like
> 'topEventLoop', and also fields that are only needed by the main thread (like
> the 'workers' registy).  Perhaps have separate structures for each of these? 
> (e.g. GlobalState,   ThreadState, and MainThreadState)

yeah - this is a nice suggestion. I'm hope that we only have 2 - thread/worker
and main-thread.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode57
frog/lib/isolate.dart:57: IsolateContext currentIsolate = null;
On 2011/11/16 19:09:17, mattsh wrote:
> isolateContext

Done

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode60
frog/lib/isolate.dart:60: IsolateContext rootIsolate = null;
On 2011/11/16 19:09:17, mattsh wrote:
> rootContext

Done.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode66
frog/lib/isolate.dart:66: bool inWorker;
On 2011/11/16 19:09:17, mattsh wrote:
> inWorker suggests this value might change.  Perhaps calls it 'isWorker'?

Done.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcod...
frog/lib/isolate.dart:249: static Function _platformDefer() native """
On 2011/11/16 19:09:17, mattsh wrote:
> platformDefer is kind of vague, suggest rename to _setTimeout (which can be
null
> if the underlying support is not available).  

Done.

[Jennifer Messerly, 2011-11-17 02:28:11]

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode78
frog/lib/isolate.dart:78: bool get useWorkers() => supportsWorkers;
a lot of these properties are redundant?
Should this have a setter (given the comment says "set to false...")?

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode85
frog/lib/isolate.dart:85: bool get useWorkerSerializationProtocol() =>
useWorkers;
Was this an old name? It's reeeeeeallllllyyyy long :)

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcod...
frog/lib/isolate.dart:117: IsolateNatives._processWorkerMessage(this.mainWorker,
e);
does the compiler know to emit this if _nativeInit is called?

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart
File frog/value.dart (right):

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart#newcode305
frog/value.dart:305: // possible.
I still don't think this is special logic. :)

All native libraries with callbacks--including node.js--will need to be isolate
aware. It is a fundamental property of implementing Dart isolates on top of a
non-isolate-aware JS runtime. And isolates are a big feature of Dart, so having
the compiler know about them is not a bad thing!

That said--it would be nice to make this more general and not DOM specific. 

What I'd like to see happen: we wrap any "native" callback automatically. The
trick is we need a way to not wrap for cases like List.sort, where we know that
the list function isn't going to call back to us from a different isolate. So we
need a way to mark the callback parameter as being same-isolate vs
different-isolate. (It'd be really nice to have more metadata in our "native"
keyword)

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart#newcode308
frog/value.dart:308: world.gen.corejs.useWrap0 = true;
should this also set useIsolates to true? Or you could end up emitting a call to
$wrap_call$1, but not emitting the function, if I'm reading it right

[Siggi Cherem (dart-lang), 2011-11-17 16:50:06]

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode78
frog/lib/isolate.dart:78: bool get useWorkers() => supportsWorkers;
On 2011/11/17 02:28:11, John Messerly wrote:
> a lot of these properties are redundant?
> Should this have a setter (given the comment says "set to false...")?

For now the 'set to false' is only done by modifying the code by hand. 

A lot of these properties I copied over from the JS version, I was trying not to
change too much how things were initially implemented.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcode85
frog/lib/isolate.dart:85: bool get useWorkerSerializationProtocol() =>
useWorkers;
On 2011/11/17 02:28:11, John Messerly wrote:
> Was this an old name? It's reeeeeeallllllyyyy long :)

I know :) - copied from JS. I'll think of something shorter.

http://codereview.chromium.org/8577003/diff/3010/frog/lib/isolate.dart#newcod...
frog/lib/isolate.dart:117: IsolateNatives._processWorkerMessage(this.mainWorker,
e);
On 2011/11/17 02:28:11, John Messerly wrote:
> does the compiler know to emit this if _nativeInit is called?

It doesn't - fortunately that method is also reachable from 'spawnWorker' below.

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart
File frog/value.dart (right):

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart#newcode300
frog/value.dart:300: * callbacks are executed withing the context of the isolate
that created them
On 2011/11/16 18:00:45, jimhug wrote:
> Nit: within

Done.

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart#newcode305
frog/value.dart:305: // possible.
On 2011/11/17 02:28:11, John Messerly wrote:
> I still don't think this is special logic. :)
> 
> All native libraries with callbacks--including node.js--will need to be
isolate
> aware. It is a fundamental property of implementing Dart isolates on top of a
> non-isolate-aware JS runtime. And isolates are a big feature of Dart, so
having
> the compiler know about them is not a bad thing!
> 
> That said--it would be nice to make this more general and not DOM specific. 
> 
> What I'd like to see happen: we wrap any "native" callback automatically. The
> trick is we need a way to not wrap for cases like List.sort, where we know
that
> the list function isn't going to call back to us from a different isolate. So
we
> need a way to mark the callback parameter as being same-isolate vs
> different-isolate. (It'd be really nice to have more metadata in our "native"
> keyword)

totally agree. Although I would be quite happy if we can implement isolates
without the need of wrapping - I'm exploring one implementation of this, but I
still need to measure its performance implications.

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart#newcode308
frog/value.dart:308: world.gen.corejs.useWrap0 = true;
On 2011/11/17 02:28:11, John Messerly wrote:
> should this also set useIsolates to true? Or you could end up emitting a call
to
> $wrap_call$1, but not emitting the function, if I'm reading it right

In this case no, we don't want to mark useIsolates=true to avoid introducing the
isolate code only because you use a callback in the dom. 

Instead, corejs.dart generates code whenever useWrap0 is true. If
useIsolates=false then the $wrap_call$0 body is just the identity function.

[Jennifer Messerly, 2011-11-17 19:18:03]

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart
File frog/value.dart (right):

http://codereview.chromium.org/8577003/diff/3010/frog/value.dart#newcode308
frog/value.dart:308: world.gen.corejs.useWrap0 = true;
On 2011/11/17 16:50:11, sigmund wrote:
> On 2011/11/17 02:28:11, John Messerly wrote:
> > should this also set useIsolates to true? Or you could end up emitting a
call
> to
> > $wrap_call$1, but not emitting the function, if I'm reading it right
> 
> In this case no, we don't want to mark useIsolates=true to avoid introducing
the
> isolate code only because you use a callback in the dom. 
> 
> Instead, corejs.dart generates code whenever useWrap0 is true. If
> useIsolates=false then the $wrap_call$0 body is just the identity function.

My bad. I didn't read corejs.dart carefully enough. I really like the approach
you took with this!