Isolates in frog - tweaks in existing js code to make things run

Isolates in frog - tweaks in existing js code to make it run. I plan to do an
actual cleanup on a second CL.

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

[Siggi Cherem (dart-lang), 2011-11-08 01:25:14]

This is only a first step:
- it makes the existing js code work to the point of running battleship
- I plan to do a follow up CL where I focus on cleaning up the code a bit more
- This doesn't address anything about on globals and keeping the generated code
truly isolated.

http://codereview.chromium.org/8467034/diff/6001/frog/gen.dart
File frog/gen.dart (right):

http://codereview.chromium.org/8467034/diff/6001/frog/gen.dart#newcode53
frog/gen.dart:53: writer.writeln('RunEntry(function () {${mainCall.code};},
[]);');
this will change again as I clean up the isolate API on a follow up CL.

http://codereview.chromium.org/8467034/diff/6001/frog/lib/core.js
File frog/lib/core.js (right):

http://codereview.chromium.org/8467034/diff/6001/frog/lib/core.js#newcode351
frog/lib/core.js:351: obj[name] = method || methods['Object'];
this change is not necessary, but something john noticed while debugging earlier
today

[Siggi Cherem (dart-lang), 2011-11-08 02:06:26]

http://codereview.chromium.org/8467034/diff/9001/frog/value.dart
File frog/value.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/value.dart#newcode220
frog/value.dart:220: return _isDomCallback(toType) && !_isDomCallback(type) ?
BTW, this was a cool trick that John came up with. but I'm not very happy having
this as part of the compiler.

We have to first reconsider the design issue of whether or not we allow multiple
isolates to have access to the DOM, and if we do, is there a way to handle at
the library level?

[Jennifer Messerly, 2011-11-08 05:04:35]

lgtm

http://codereview.chromium.org/8467034/diff/9001/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/lib/corelib_impl.dart#n...
frog/lib/corelib_impl.dart:29: // trying to get things compiling again.
This TODO can go :) ... I'll remove it in my upcoming change

http://codereview.chromium.org/8467034/diff/9001/frog/lib/corelib_impl.dart#n...
frog/lib/corelib_impl.dart:30: #native("isolate.js");
I noticed this causes pretty bad code bloat. Something to think about is how we
can get inject this JS only when the isolates are actually used.

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

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.dart#newcode11
frog/lib/isolate.dart:11: throw "SendPort::send: Illegal replyTo type.";
probably want to throw an actual exception type?

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.dart#newcode17
frog/lib/isolate.dart:17: // TODO(sigmund): get rid of _sendNow
I think you can just remove it? I didn't see any callers.

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js
File frog/lib/isolate.js (right):

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js#newcode253
frog/lib/isolate.js:253: // Both, the message and the replyTo are already
serialized.
nit: extra comma

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js#newcode256
frog/lib/isolate.js:256: if (!isolate) return;  // Isolate has been closed.
are either of these conditions errors?

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js#newcode284
frog/lib/isolate.js:284: });
can this just be isolate.run(self) ?

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js#newcode299
frog/lib/isolate.js:299: });
nit: i'd put on one line:

isolate.run(function() { self(arg); });

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate_serializati...
File frog/lib/isolate_serialization.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate_serializati...
frog/lib/isolate_serialization.dart:7: * isolates.
I'd add a TODO: we should get rid of a lot of this serialization. It seems to
have a big overlap with what JSON already does.

http://codereview.chromium.org/8467034/diff/9001/frog/value.dart
File frog/value.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/value.dart#newcode220
frog/value.dart:220: return _isDomCallback(toType) && !_isDomCallback(type) ?
On 2011/11/08 02:06:26, sigmund wrote:
> BTW, this was a cool trick that John came up with. but I'm not very happy
having
> this as part of the compiler.
> 
> We have to first reconsider the design issue of whether or not we allow
multiple
> isolates to have access to the DOM, and if we do, is there a way to handle at
> the library level?

It'd be worth putting the "we have to first reconsider..." as a TODO.

In general I don't think there's anything wrong with isolates & the DOM being a
bit special. What would be better though is if we had a generic way of marking
function-typed parameters as "native callbacks". Then the "conversion" only
applies to those parameters. (Converting arguments when going from
dynamic->static is a common trick in many systems. Usually you specify it with
some sort of metadata, e.g. the C# [MarshalAs] attribute)

I wonder if "typedefs" allow "native" in the grammar? That might give us the
hook we need to put it in the definition of EventListener & friends.

[jimhug, 2011-11-08 15:39:00]

LGTM!

This came in far better than I'd hoped for a first commit of isolates.  I'm
looking forward to see where this goes from here!

http://codereview.chromium.org/8467034/diff/9001/client/samples/dartcombat/vi...
File client/samples/dartcombat/views.dart (right):

http://codereview.chromium.org/8467034/diff/9001/client/samples/dartcombat/vi...
client/samples/dartcombat/views.dart:285: completer.complete([x, y]);
Why did you lose the const here?

http://codereview.chromium.org/8467034/diff/9001/frog/frogsh
File frog/frogsh (right):

http://codereview.chromium.org/8467034/diff/9001/frog/frogsh#newcode19176
frog/frogsh:19176: RunEntry(function () {main();}, []);
500 lines is a lot to add - but considering that we get a feature as big as
isolates in return and that we have plans to reduce this overhead with time I
think this is a fair price to pay.

http://codereview.chromium.org/8467034/diff/9001/frog/gen.dart
File frog/gen.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/gen.dart#newcode53
frog/gen.dart:53: writer.writeln('RunEntry(function () {${mainCall.code};},
[]);');
Does this cause any issues with our stack traces?

http://codereview.chromium.org/8467034/diff/9001/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/lib/corelib_impl.dart#n...
frog/lib/corelib_impl.dart:30: #native("isolate.js");
I agree that the code bloat is annoying - but I talked with Siggi about this
yesterday and should point out that I was the one who encouraged him to check in
the first working version of this ASAP and then make incremental improvements
from there.  I think this is exactly the right path and look forward to seeing
the code get steadily cleaner over the course of the week.

On 2011/11/08 05:04:35, John Messerly wrote:
> I noticed this causes pretty bad code bloat. Something to think about is how
we
> can get inject this JS only when the isolates are actually used.

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

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js#oldcode5
frog/lib/isolate.js:5: // TODO(jmesserly): this whole file won't work in Frog
yet.
Yay!  Nice TODO to make go away.

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js#oldcode13
frog/lib/isolate.js:13: // optimizer. They are defined in
client/dom/generated/dart_dom_wrapping.js.
<smile>

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate_serializati...
File frog/lib/isolate_serialization.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate_serializati...
frog/lib/isolate_serialization.dart:7: * isolates.
+1 - But I'm also a little concerned about how we will handle serialization
between VM and JS implemented isolates, not to mention from client to server.  I
hope we can do that without a lot of custom serialization code in the JS-based
clients where code size will be most critical.
On 2011/11/08 05:04:35, John Messerly wrote:
> I'd add a TODO: we should get rid of a lot of this serialization. It seems to
> have a big overlap with what JSON already does.

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate_serializati...
frog/lib/isolate_serialization.dart:70:
"o['__MessageTraverser__attached_info__'] = (void 0);";
I'm going to have to get you to explain (void 0) to me later today.  I've never
seen it before and can't quite work out what it means.

http://codereview.chromium.org/8467034/diff/9001/frog/value.dart
File frog/value.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/value.dart#newcode220
frog/value.dart:220: return _isDomCallback(toType) && !_isDomCallback(type) ?
I'm not sure about the right long-term solution, but like this as a hack for
now.  +1 to the suggestion to add a TODO with the contents of your comment
above.
On 2011/11/08 05:04:35, John Messerly wrote:
> On 2011/11/08 02:06:26, sigmund wrote:
> > BTW, this was a cool trick that John came up with. but I'm not very happy
> having
> > this as part of the compiler.
> > 
> > We have to first reconsider the design issue of whether or not we allow
> multiple
> > isolates to have access to the DOM, and if we do, is there a way to handle
at
> > the library level?
> 
> It'd be worth putting the "we have to first reconsider..." as a TODO.
> 
> In general I don't think there's anything wrong with isolates & the DOM being
a
> bit special. What would be better though is if we had a generic way of marking
> function-typed parameters as "native callbacks". Then the "conversion" only
> applies to those parameters. (Converting arguments when going from
> dynamic->static is a common trick in many systems. Usually you specify it with
> some sort of metadata, e.g. the C# [MarshalAs] attribute)
> 
> I wonder if "typedefs" allow "native" in the grammar? That might give us the
> hook we need to put it in the definition of EventListener & friends.

http://codereview.chromium.org/8467034/diff/9001/frog/value.dart#newcode281
frog/value.dart:281: bool _isDomCallback(toType) {
This could use a doc comment and possibly a TODO as well to highlight the
deliberate choice to do something a little weird for a compiler - in return for
great benefit as a web language.

http://codereview.chromium.org/8467034/diff/9001/tests/isolate/isolate.status
File tests/isolate/isolate.status (right):

http://codereview.chromium.org/8467034/diff/9001/tests/isolate/isolate.status...
tests/isolate/isolate.status:24: [ $component == frog || $component == frogsh ||
$component == leg ]
The small number of Fails here makes me suspicious <smile>.

http://codereview.chromium.org/8467034/diff/9001/tests/language/language.status
File tests/language/language.status (left):

http://codereview.chromium.org/8467034/diff/9001/tests/language/language.stat...
tests/language/language.status:315: TypedMessageTest: Fail
Your changes really fixed this?  Does this mean this test should get moved to
isolate?

[Siggi Cherem (dart-lang), 2011-11-08 17:56:42]

http://codereview.chromium.org/8467034/diff/9001/client/samples/dartcombat/vi...
File client/samples/dartcombat/views.dart (right):

http://codereview.chromium.org/8467034/diff/9001/client/samples/dartcombat/vi...
client/samples/dartcombat/views.dart:285: completer.complete([x, y]);
On 2011/11/08 15:39:01, jimhug wrote:
> Why did you lose the const here?

Basically x, y are variables, which are not allowed to be used within const
expressions. We actually want here an immutable array (rather than a const).

http://codereview.chromium.org/8467034/diff/9001/frog/frogsh
File frog/frogsh (right):

http://codereview.chromium.org/8467034/diff/9001/frog/frogsh#newcode19176
frog/frogsh:19176: RunEntry(function () {main();}, []);
On 2011/11/08 15:39:01, jimhug wrote:
> 500 lines is a lot to add - but considering that we get a feature as big as
> isolates in return and that we have plans to reduce this overhead with time I
> think this is a fair price to pay.

Yeah - I'll be working this week on bringing this down. Hopefully to something
near-zero if you are not using any isolates in your code.

http://codereview.chromium.org/8467034/diff/9001/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/lib/corelib_impl.dart#n...
frog/lib/corelib_impl.dart:29: // trying to get things compiling again.
On 2011/11/08 05:04:35, John Messerly wrote:
> This TODO can go :) ... I'll remove it in my upcoming change

:)

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

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.dart#newcode11
frog/lib/isolate.dart:11: throw "SendPort::send: Illegal replyTo type.";
On 2011/11/08 05:04:35, John Messerly wrote:
> probably want to throw an actual exception type?

will do - the current throw was basically copied over from the JS code.

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.dart#newcode17
frog/lib/isolate.dart:17: // TODO(sigmund): get rid of _sendNow
On 2011/11/08 05:04:35, John Messerly wrote:
> I think you can just remove it? I didn't see any callers.

Unfortunately some other code in corelib is still using this, I'll remove it
after I clean up that code.

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js
File frog/lib/isolate.js (right):

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js#newcode253
frog/lib/isolate.js:253: // Both, the message and the replyTo are already
serialized.
On 2011/11/08 05:04:35, John Messerly wrote:
> nit: extra comma

copy/paste :) - fixed.

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js#newcode284
frog/lib/isolate.js:284: });
On 2011/11/08 05:04:35, John Messerly wrote:
> can this just be isolate.run(self) ?

good point. Done

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate.js#newcode299
frog/lib/isolate.js:299: });
On 2011/11/08 05:04:35, John Messerly wrote:
> nit: i'd put on one line:
> 
> isolate.run(function() { self(arg); });

Done.

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate_serializati...
File frog/lib/isolate_serialization.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/lib/isolate_serializati...
frog/lib/isolate_serialization.dart:70:
"o['__MessageTraverser__attached_info__'] = (void 0);";
On 2011/11/08 15:39:01, jimhug wrote:
> I'm going to have to get you to explain (void 0) to me later today.  I've
never
> seen it before and can't quite work out what it means.

void is an operator that produces the side effects of the following expression
(in this case there are none), and then returns 'undefined'. 

It's basically a way to obtain undefined, even when the 'undefined' symbol is
overwritten.

http://codereview.chromium.org/8467034/diff/9001/frog/value.dart
File frog/value.dart (right):

http://codereview.chromium.org/8467034/diff/9001/frog/value.dart#newcode220
frog/value.dart:220: return _isDomCallback(toType) && !_isDomCallback(type) ?
On 2011/11/08 15:39:01, jimhug wrote:
> I'm not sure about the right long-term solution, but like this as a hack for
> now.  +1 to the suggestion to add a TODO with the contents of your comment
> above.
> On 2011/11/08 05:04:35, John Messerly wrote:
> > On 2011/11/08 02:06:26, sigmund wrote:
> > > BTW, this was a cool trick that John came up with. but I'm not very happy
> > having
> > > this as part of the compiler.
> > > 
> > > We have to first reconsider the design issue of whether or not we allow
> > multiple
> > > isolates to have access to the DOM, and if we do, is there a way to handle
> at
> > > the library level?
> > 
> > It'd be worth putting the "we have to first reconsider..." as a TODO.
> > 
> > In general I don't think there's anything wrong with isolates & the DOM
being
> a
> > bit special. What would be better though is if we had a generic way of
marking
> > function-typed parameters as "native callbacks". Then the "conversion" only
> > applies to those parameters. (Converting arguments when going from
> > dynamic->static is a common trick in many systems. Usually you specify it
with
> > some sort of metadata, e.g. the C# [MarshalAs] attribute)
> > 
> > I wonder if "typedefs" allow "native" in the grammar? That might give us the
> > hook we need to put it in the definition of EventListener & friends.
> 

Done

http://codereview.chromium.org/8467034/diff/9001/frog/value.dart#newcode281
frog/value.dart:281: bool _isDomCallback(toType) {
On 2011/11/08 15:39:01, jimhug wrote:
> This could use a doc comment and possibly a TODO as well to highlight the
> deliberate choice to do something a little weird for a compiler - in return
for
> great benefit as a web language.

Done.

http://codereview.chromium.org/8467034/diff/9001/tests/language/language.status
File tests/language/language.status (left):

http://codereview.chromium.org/8467034/diff/9001/tests/language/language.stat...
tests/language/language.status:315: TypedMessageTest: Fail
On 2011/11/08 15:39:01, jimhug wrote:
> Your changes really fixed this?  Does this mean this test should get moved to
> isolate?

I believe so, yes. This test basically creates isolates and sends messages
across.