Support web workers in dart2js.

Support web workers in dart2js.
Committed: https://code.google.com/p/dart/source/detail?r=16155

[ngeoffray, 2012-12-12 15:37:21]

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (left):

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:8: import
'dart:uri';
For probably bootstrapping problems, having this import messes up the import
combinators (test language/import_combinator_test.dart).

[kasperl, 2012-12-13 08:45:11]

LGTM.

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (left):

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1791: //
TODO(ngeoffray): These globals are currently required by the isolate
Very happy to see this get removed.

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:167: ||
(isWorkerDefined && IsolateNatives._thisScript != null);
So this means that if you can't find the script you'll fall back to not using
workers? Cool!

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:294: if
(JS('String', 'typeof window') != 'undefined') {
It would be faster to do: JS('bool', 'typeof window != "undefined"').

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:379: static
var globalThis = JS('', 'function() { return this; }()');
How about using lazy initialized statics to cache the results of globalThis and
_thisScript? Rename the current methods to computeThisScript and
computeGlobalThis and do:

   static var globalThis = computeGlobalThis();
   static String thisScript = computeThisScript();

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:386: //
TODO(5334778): Find a cross-platform non-brittle way of getting the
File a new bug on dartbug.com.

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1120: // Fetch the
current isolate in a local variable to prevent the
Extend comment to mention that the current isolate may have changed so if we
read it from within the JS closure it leads to the wrong thing.

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/html/dart2js/html_...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/html/dart2js/html_...
sdk/lib/html/dart2js/html_dart2js.dart:49: // TODO(vsm): Plumb this properly.
Remove the TODO?

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/html/templates/htm...
File sdk/lib/html/templates/html/dart2js/html_dart2js.darttemplate (right):

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/html/templates/htm...
sdk/lib/html/templates/html/dart2js/html_dart2js.darttemplate:73: // TODO(vsm):
Plumb this properly.
Remove TODO?

https://codereview.chromium.org/11553024/diff/4002/tests/isolate/isolate2_neg...
File tests/isolate/isolate2_negative_test.dart (right):

https://codereview.chromium.org/11553024/diff/4002/tests/isolate/isolate2_neg...
tests/isolate/isolate2_negative_test.dart:17: port.receive((msg) {});
Why did you add this? Maybe add a comment.

[ngeoffray, 2012-12-13 11:56:13]

Thanks Kasper.

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:167: ||
(isWorkerDefined && IsolateNatives._thisScript != null);
On 2012/12/13 08:45:12, kasperl wrote:
> So this means that if you can't find the script you'll fall back to not using
> workers? Cool!

Yes :)

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:294: if
(JS('String', 'typeof window') != 'undefined') {
On 2012/12/13 08:45:12, kasperl wrote:
> It would be faster to do: JS('bool', 'typeof window != "undefined"').

Faster in which way? We generate the same code for both versions.

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:379: static
var globalThis = JS('', 'function() { return this; }()');
On 2012/12/13 08:45:12, kasperl wrote:
> How about using lazy initialized statics to cache the results of globalThis
and
> _thisScript? Rename the current methods to computeThisScript and
> computeGlobalThis and do:
> 
>    static var globalThis = computeGlobalThis();
>    static String thisScript = computeThisScript();

Done.

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:386: //
TODO(5334778): Find a cross-platform non-brittle way of getting the
On 2012/12/13 08:45:12, kasperl wrote:
> File a new bug on http://dartbug.com.

Done: http://code.google.com/p/dart/issues/detail?id=7369.

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1120: // Fetch the
current isolate in a local variable to prevent the
On 2012/12/13 08:45:12, kasperl wrote:
> Extend comment to mention that the current isolate may have changed so if we
> read it from within the JS closure it leads to the wrong thing.

Done.

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/html/dart2js/html_...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/html/dart2js/html_...
sdk/lib/html/dart2js/html_dart2js.dart:49: // TODO(vsm): Plumb this properly.
On 2012/12/13 08:45:12, kasperl wrote:
> Remove the TODO?

Done.

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/html/templates/htm...
File sdk/lib/html/templates/html/dart2js/html_dart2js.darttemplate (right):

https://codereview.chromium.org/11553024/diff/4002/sdk/lib/html/templates/htm...
sdk/lib/html/templates/html/dart2js/html_dart2js.darttemplate:73: // TODO(vsm):
Plumb this properly.
On 2012/12/13 08:45:12, kasperl wrote:
> Remove TODO?

Done.

https://codereview.chromium.org/11553024/diff/4002/tests/isolate/isolate2_neg...
File tests/isolate/isolate2_negative_test.dart (right):

https://codereview.chromium.org/11553024/diff/4002/tests/isolate/isolate2_neg...
tests/isolate/isolate2_negative_test.dart:17: port.receive((msg) {});
On 2012/12/13 08:45:12, kasperl wrote:
> Why did you add this? Maybe add a comment.

Because otherwise the main isolate quits and the spawned isolate does not have a
chance to execute. I added a comment.

[floitsch, 2012-12-13 13:06:38]

dbc.

https://codereview.chromium.org/11553024/diff/16002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/foreign_helper.dart (right):

https://codereview.chromium.org/11553024/diff/16002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/foreign_helper.dart:16: * Return
[:true:] if [object] has its own operator== definition.
Why this change?
Method comments should start with a third-person verb.

https://codereview.chromium.org/11553024/diff/16002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11553024/diff/16002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:160: bool
isWindowDefined = JS("String", r"typeof window") != 'undefined';
In theory this is dangerous. There is no guarantee that the namer doesn't
generate a 'window' local.
Maybe use the globalThis?
JS("String", r"typeof #['postMessage']", globalThis)

[ngeoffray, 2012-12-13 16:03:53]

Thanks Florian.

Florian and Kasper, PTAL at patchset #4. I needed to fix some code to make all
tests pass.

https://codereview.chromium.org/11553024/diff/16002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/foreign_helper.dart (right):

https://codereview.chromium.org/11553024/diff/16002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/foreign_helper.dart:16: * Return
[:true:] if [object] has its own operator== definition.
On 2012/12/13 13:06:38, floitsch wrote:
> Why this change?
> Method comments should start with a third-person verb.

Done.

https://codereview.chromium.org/11553024/diff/16002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11553024/diff/16002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:160: bool
isWindowDefined = JS("String", r"typeof window") != 'undefined';
On 2012/12/13 13:06:38, floitsch wrote:
> In theory this is dangerous. There is no guarantee that the namer doesn't
> generate a 'window' local.
> Maybe use the globalThis?
> JS("String", r"typeof #['postMessage']", globalThis)

Done.

[kasperl, 2012-12-14 08:16:35]

I think we need to work a bit more on the close down sequence.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:187: // Make
sure the worker does not process any new messages.
This shouldn't be necessary if you only close a worker that has no open receive
ports.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:274:
_globalState.maybeCloseWorker();
This looks fishy. It seems like we should only close a worker if it has no open
receive ports and no pending events.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:386: var
globalPostMessage = JS('', "#['postMessage']", globalThis);
I'd change this to be a bool (globalPostMessageDefined) because I don't like
having a Dart variable refer to a raw JS closure.

[ahe, 2012-12-14 08:33:31]

LGTM except for
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart.  I don't
understand that file, but I think you have other reviewers that do.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:470: // The helper
library does not use the native language extension,
The *isolate* helper library...

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:487:
importHelperLibrary(isolateHelperLibrary);
importHelperLibrary means "import *JS* helper library".  So "helper library"
means "JS helper library".  I find this confusing, and I think you do too.  So
perhaps this method should be renamed.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:43: // by
having a "default" isolate (the first one created).
Is this still the case, and do we have a bug to track it?  The bug number above
looks like an internal Google bug number.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:81: // the
change to break the isolate abstraction.
Not your code: change -> chance

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:82: _Manager
get _globalState => JS("_Manager", r"$globalState");
Shouldn't this be something like $.globalState?

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:85: JS("void",
r"$globalState = #", val);
Ditto.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1126: function =
JS("var", r"""function() {
Why doesn't this function take two arguments?

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1126: function =
JS("var", r"""function() {
I think it would be cleaner if this expression was moved to its own function,
like this:

function = buildJsClosure(DART_CLOSURE_TO_JS(invokeClosure),
    closure,
    JS_CURRENT_ISOLATE(),
    arity);

buildJsClosure(jsInvokeClosure, closure, currentIsolate, arity) {
  return JS('var' ,'function(a, b) { return #(#, #, #, a, b); }',
      jsInvokeClosure, closure, currentIsolate, arity);
}

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1129:
DART_CLOSURE_TO_JS(invokeClosure),
Weird indentation.

https://codereview.chromium.org/11553024/diff/13007/tests/isolate/isolate2_ne...
File tests/isolate/isolate2_negative_test.dart (right):

https://codereview.chromium.org/11553024/diff/13007/tests/isolate/isolate2_ne...
tests/isolate/isolate2_negative_test.dart:17: // Make sure the main thread stays
alive so that the spawned isolate
I'm not sure about this.  Why does the main thread not stay alive until you
close port?

[kasperl, 2012-12-14 08:47:23]

Hmm. I hope we figured something out here. I'd argue that renaming globalState
to isolateState or workerState or something would make it easier to understand.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:445:
msg['port'].send(msg['msg'], msg['replyTo']);
This is where we want to check that msg['port'] isn't null. If it is null we
should ignore the message.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:973: var
receivePort = isolate.lookup(receivePortId);
Could this return null (if the receive port had been closed)? Should we return a
null SendPort in that case too?

[ngeoffray, 2012-12-14 09:13:16]

Thanks Peter and Kasper.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:43: // by
having a "default" isolate (the first one created).
On 2012/12/14 08:33:31, ahe wrote:
> Is this still the case, and do we have a bug to track it?  The bug number
above
> looks like an internal Google bug number.

I think you're right, we can remove this restriction. I'll clean this up.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:81: // the
change to break the isolate abstraction.
On 2012/12/14 08:33:31, ahe wrote:
> Not your code: change -> chance

Done.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:82: _Manager
get _globalState => JS("_Manager", r"$globalState");
On 2012/12/14 08:33:31, ahe wrote:
> Shouldn't this be something like $.globalState?

Ideally yes. But it does not work right now because of how this code sets up
static variables. Again, I plan to address this.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:187: // Make
sure the worker does not process any new messages.
On 2012/12/14 08:16:35, kasperl wrote:
> This shouldn't be necessary if you only close a worker that has no open
receive
> ports.

Done.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:386: var
globalPostMessage = JS('', "#['postMessage']", globalThis);
On 2012/12/14 08:16:35, kasperl wrote:
> I'd change this to be a bool (globalPostMessageDefined) because I don't like
> having a Dart variable refer to a raw JS closure.

Done.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:445:
msg['port'].send(msg['msg'], msg['replyTo']);
On 2012/12/14 08:47:23, kasperl wrote:
> This is where we want to check that msg['port'] isn't null. If it is null we
> should ignore the message.

Done.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:973: var
receivePort = isolate.lookup(receivePortId);
On 2012/12/14 08:47:23, kasperl wrote:
> Could this return null (if the receive port had been closed)? Should we return
a
> null SendPort in that case too? 

Good catch. Fixed.

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11553024/diff/13007/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1126: function =
JS("var", r"""function() {
On 2012/12/14 08:33:31, ahe wrote:
> Why doesn't this function take two arguments?

That's a good question. I'll address it in a different CL.

https://codereview.chromium.org/11553024/diff/13007/tests/isolate/isolate2_ne...
File tests/isolate/isolate2_negative_test.dart (right):

https://codereview.chromium.org/11553024/diff/13007/tests/isolate/isolate2_ne...
tests/isolate/isolate2_negative_test.dart:17: // Make sure the main thread stays
alive so that the spawned isolate
On 2012/12/14 08:33:31, ahe wrote:
> I'm not sure about this.  Why does the main thread not stay alive until you
> close port?

Hmmm. Good catch. I added a TODO.

[floitsch, 2012-12-14 10:25:27]

dbc.

https://codereview.chromium.org/11553024/diff/17014/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11553024/diff/17014/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:384: JS('',
"#['postMessage'] !== (void 0)", globalThis);
You could add a type to the JS.