Implement scheduleImmediate for the VM.

Implement scheduleImmediate for the VM.

BUG= http://dartbug.com/9001
R=iposva@google.com

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

[floitsch, 2013-11-20 19:37:13]

Minor things are still missing (added comments for the things that I still want
to change), but otherwise this CL should be ready.

https://codereview.chromium.org/79243002/diff/1/runtime/lib/isolate_patch.dart
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/79243002/diff/1/runtime/lib/isolate_patch.dar...
runtime/lib/isolate_patch.dart:60: var _pendingImmediateCallback;
Not yet typed. Will do that.

https://codereview.chromium.org/79243002/diff/1/runtime/lib/isolate_patch.dar...
runtime/lib/isolate_patch.dart:65: Function
_getIsolateScheduleImmediateClosure() {
ditto.

https://codereview.chromium.org/79243002/diff/1/runtime/lib/schedule_microtas...
File runtime/lib/schedule_microtask_patch.dart (right):

https://codereview.chromium.org/79243002/diff/1/runtime/lib/schedule_microtas...
runtime/lib/schedule_microtask_patch.dart:5: @deprecated
I need to update Dartium before I can remove this function.
Once this CL is committed it is a trivial patch to switch Dartium (without ever
going red on the buildbot).

[floitsch, 2013-11-21 17:39:47]

https://codereview.chromium.org/79243002/diff/1/runtime/lib/isolate_patch.dart
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/79243002/diff/1/runtime/lib/isolate_patch.dar...
runtime/lib/isolate_patch.dart:60: var _pendingImmediateCallback;
On 2013/11/20 19:37:13, floitsch wrote:
> Not yet typed. Will do that.

Done.

https://codereview.chromium.org/79243002/diff/1/runtime/lib/isolate_patch.dar...
runtime/lib/isolate_patch.dart:65: Function
_getIsolateScheduleImmediateClosure() {
On 2013/11/20 19:37:13, floitsch wrote:
> ditto.

In the end didn't type it. It's only used for the embedder and I didn't want to
add yet another typedef.

https://codereview.chromium.org/79243002/diff/90001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/79243002/diff/90001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:637: if (element.name ==
'_createTimer' ||
_createTimer, _createPeriodicTimer and scheduleImmediate (below) all use the
TimerImpl from the isolate_helper library, but I wasn't able to simplify the
code to just look for that.

[floitsch, 2013-11-25 16:03:37]

ping.

I updated the (deprecated) schedule_microtask_patch.dart file so that it
continues working with the current Dartium code.

See https://chromiumcodereview.appspot.com/85333004/ for the patch to Dartium.
With that patch we can completely remove schedule_microtask_patch.dart.

[floitsch, 2013-11-27 13:58:54]

More than 6 days now...
Ping.

[Ivan Posva, 2013-11-27 22:08:32]

LGTM with comments addressed.

-Ivan

https://codereview.chromium.org/79243002/diff/160001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/79243002/diff/160001/runtime/bin/dartutils.cc...
runtime/bin/dartutils.cc:31: const char* DartUtils::kInternalLibURL =
"dart:_collection-dev";
The name does not match. If we are starting to use dart:_collection_dev for all
kinds of code unrelated to collections, then we should rename it. But here the
names should be in sync.

https://codereview.chromium.org/79243002/diff/160001/runtime/bin/dartutils.cc...
runtime/bin/dartutils.cc:672: Dart_Handle url = NewString(kInternalLibURL);
Can you please move these lines dealing with dart:_collection_dev below the
lookup of the print closure? It gets very confusing as it almost looks like we
are calling the _getPrintClosure in dart:_collection_dev and not in
dart:builtin.

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:74: Function get _isolateScheduleImmediateClosure
=> _isolateScheduleImmediate;
There is one too many ways to get at the closure. Please remove this as it is
more confusing than is worth. All the other embedder defined closures are of the
form _getXYZClosure().

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/schedule_imm...
File runtime/lib/schedule_immediate_patch.dart (right):

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/schedule_imm...
runtime/lib/schedule_immediate_patch.dart:14: ScheduleImmediateClosure
scheduleImmediateClosure;
Why is this public?

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/schedule_mic...
File runtime/lib/schedule_microtask_patch.dart (right):

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/schedule_mic...
runtime/lib/schedule_microtask_patch.dart:5: @deprecated
Who is relying on this and what is the schedule of removing this outdated
mechanism?

https://codereview.chromium.org/79243002/diff/160001/sdk/lib/_internal/lib/co...
File sdk/lib/_internal/lib/collection_dev_patch.dart (right):

https://codereview.chromium.org/79243002/diff/160001/sdk/lib/_internal/lib/co...
sdk/lib/_internal/lib/collection_dev_patch.dart:17: patch void
scheduleImmediate(void callback()) {
Wasn't the goal of this CL to get rid of the reliance on a timer?

[floitsch, 2013-11-28 19:05:54]

Thanks Ivan.

Your comments made me realize that I don't need to move the scheduleImmediate
into the collection-dev library anymore.
This simplified the code.
PTAL. (it should go fast).

https://codereview.chromium.org/79243002/diff/160001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/79243002/diff/160001/runtime/bin/dartutils.cc...
runtime/bin/dartutils.cc:31: const char* DartUtils::kInternalLibURL =
"dart:_collection-dev";
On 2013/11/27 22:08:32, Ivan Posva wrote:
> The name does not match. If we are starting to use dart:_collection_dev for
all
> kinds of code unrelated to collections, then we should rename it. But here the
> names should be in sync.

Done.

https://codereview.chromium.org/79243002/diff/160001/runtime/bin/dartutils.cc...
runtime/bin/dartutils.cc:672: Dart_Handle url = NewString(kInternalLibURL);
On 2013/11/27 22:08:32, Ivan Posva wrote:
> Can you please move these lines dealing with dart:_collection_dev below the
> lookup of the print closure? It gets very confusing as it almost looks like we
> are calling the _getPrintClosure in dart:_collection_dev and not in
> dart:builtin.

Done.

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:74: Function get _isolateScheduleImmediateClosure
=> _isolateScheduleImmediate;
On 2013/11/27 22:08:32, Ivan Posva wrote:
> There is one too many ways to get at the closure. Please remove this as it is
> more confusing than is worth. All the other embedder defined closures are of
the
> form _getXYZClosure().

Dartium usually uses getters, but I should be able to adapt their code.
done.

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/schedule_imm...
File runtime/lib/schedule_immediate_patch.dart (right):

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/schedule_imm...
runtime/lib/schedule_immediate_patch.dart:14: ScheduleImmediateClosure
scheduleImmediateClosure;
On 2013/11/27 22:08:32, Ivan Posva wrote:
> Why is this public?

It is inside the collection-dev library (thus not publicly visible).
However that was, so that isolates could automatically take over the microtask
queue.
Now that the embedder is responsible for it I will move it back to the
async-library and make it private again.

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/schedule_mic...
File runtime/lib/schedule_microtask_patch.dart (right):

https://codereview.chromium.org/79243002/diff/160001/runtime/lib/schedule_mic...
runtime/lib/schedule_microtask_patch.dart:5: @deprecated
On 2013/11/27 22:08:32, Ivan Posva wrote:
> Who is relying on this and what is the schedule of removing this outdated
> mechanism?

There was a comment in a earlier patchset.

I wanted isolates to take over the scheduleImmediate if nobody else was in
charge.
However we now require the embedder to set the function, and I can revert that
part of the CL.

https://codereview.chromium.org/79243002/diff/160001/sdk/lib/_internal/lib/co...
File sdk/lib/_internal/lib/collection_dev_patch.dart (right):

https://codereview.chromium.org/79243002/diff/160001/sdk/lib/_internal/lib/co...
sdk/lib/_internal/lib/collection_dev_patch.dart:17: patch void
scheduleImmediate(void callback()) {
On 2013/11/27 22:08:32, Ivan Posva wrote:
> Wasn't the goal of this CL to get rid of the reliance on a timer?

This is in the dart2js version.
This change should be gone once I revert the unnecessary changes in this CL.

[Ivan Posva, 2013-11-30 10:37:40]

Still LGTM.

-Ivan

[floitsch, 2013-12-02 16:27:32]

Committed patchset #8 manually as r30797 (presubmit successful).