Trying to make preamble for d8.

Trying to make preamble for d8.

Preamble handles timers and task queue.

R=floitsch@google.com

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

[Lasse Reichstein Nielsen, 2014-03-18 12:30:23]

PTAL.
The code almost works, but it doesn't handle non-zero timers as gracefully as
the Dart simulation.

[floitsch, 2014-03-18 13:18:22]

LGTM.

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/as...
File sdk/lib/_internal/lib/async_patch.dart (right):

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/as...
sdk/lib/_internal/lib/async_patch.dart:21: if (milliseconds > 0) print("T:
$milliseconds");
debug code. (just fyi).

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/pr...
File sdk/lib/_internal/lib/preambles/d8.js (right):

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/pr...
sdk/lib/_internal/lib/preambles/d8.js:31: if (head == tail) return;
if (!hasTask()) return;
?

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/pr...
sdk/lib/_internal/lib/preambles/d8.js:42: if (split * 2 < length) {  // split <
length / 2
I wouldn't do the optimization of copying the smaller section.
This should be rare, and small. No need to complicate the code.

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/pr...
sdk/lib/_internal/lib/preambles/d8.js:86: delete timerIds[id];
Use cancelTimer here.

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/pr...
sdk/lib/_internal/lib/preambles/d8.js:102: if (typeof onerror == "function") {
Not sure we want to introduce this already in this CL.
But ok with it.

[Lasse Reichstein Nielsen, 2014-03-19 10:24:41]

I added non-zero timer support too.
Too many tests were failing because it thought it had timer support, but it
still threw an unexpected string as error.

Now only one test is failing in lib/async: deferred/deferred_in_isolate_test.
The timeout happens immediately (because timers are simulated by advancing the
time), before the load is done. It seems the isolate doesn't use the same
queues, so the main loop terminates before the isolate has loaded the deferred
library.

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/as...
File sdk/lib/_internal/lib/async_patch.dart (right):

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/as...
sdk/lib/_internal/lib/async_patch.dart:21: if (milliseconds > 0) print("T:
$milliseconds");
On 2014/03/18 13:18:23, floitsch wrote:
> debug code. (just fyi).

Done.

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/pr...
File sdk/lib/_internal/lib/preambles/d8.js (right):

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/pr...
sdk/lib/_internal/lib/preambles/d8.js:31: if (head == tail) return;
I removed hasTask, and use return of undefined to represent emptyness.

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/pr...
sdk/lib/_internal/lib/preambles/d8.js:42: if (split * 2 < length) {  // split <
length / 2
But now it's there, so why remove it? :)

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/pr...
sdk/lib/_internal/lib/preambles/d8.js:86: delete timerIds[id];
Good idea.

https://codereview.chromium.org/201873002/diff/40001/sdk/lib/_internal/lib/pr...
sdk/lib/_internal/lib/preambles/d8.js:102: if (typeof onerror == "function") {
Let's keep it as tentative for now. It's not being used.

[floitsch, 2014-03-19 14:08:31]

LGTM.

https://codereview.chromium.org/201873002/diff/120001/sdk/lib/_internal/lib/p...
File sdk/lib/_internal/lib/preambles/d8.js (right):

https://codereview.chromium.org/201873002/diff/120001/sdk/lib/_internal/lib/p...
sdk/lib/_internal/lib/preambles/d8.js:109: now = Date.now;
I don't understand this.

https://codereview.chromium.org/201873002/diff/120001/sdk/lib/_internal/lib/p...
sdk/lib/_internal/lib/preambles/d8.js:183: if (rightChildIndex >
timerHeap.length) break;
Nit.
It's hard to see that this test checks for the leftChildIndex.

I would prefer to compute the leftChildIndex first. Then check if it's in bound.
Then +1 for the rightChild and check again.
Now we have two different checks to see if the index is in bound.

The extreme version even keeps the check for the indices the same:

var minChild, minChildIndex;
var minChildKey = key + 1;  // Aborts if no child is found.
var leftChildIndex = index * 2 + 1;
if (leftChildIndex < length) {
  minChild = ...
  ...
  var rightChildIndex = leftChildIndex + 1;
  if (rightChildIndex < length) {
}
if (minChildKey > key) break;
...

https://codereview.chromium.org/201873002/diff/120001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/201873002/diff/120001/tests/lib/lib.status#ne...
tests/lib/lib.status:189: [ $compiler == dart2js && ( $browser || $runtimer ==
d8 ) ]
runtime = d8

[Lasse Reichstein Nielsen, 2014-04-11 10:28:43]

Fix bug.

https://codereview.chromium.org/201873002/diff/120001/sdk/lib/_internal/lib/p...
File sdk/lib/_internal/lib/preambles/d8.js (right):

https://codereview.chromium.org/201873002/diff/120001/sdk/lib/_internal/lib/p...
sdk/lib/_internal/lib/preambles/d8.js:109: now = Date.now;
Comment added.

[Lasse Reichstein Nielsen, 2014-04-11 10:29:17]

Committed patchset #9 manually as r34967 (presubmit successful).

[sra1, 2014-04-11 20:23:49]

On 2014/04/11 10:29:17, Lasse Reichstein Nielsen wrote:
> Committed patchset #9 manually as r34967 (presubmit successful).

This CL, if I understand it, is to support running more tests on d8.
However, it adds 1.2k to swarm.

Is it possible to refactor it to push more of the tax into preambles/d8.js ?

[Lasse Reichstein Nielsen, 2014-04-11 22:37:39]

On 2014/04/11 20:23:49, sra1 wrote:
> On 2014/04/11 10:29:17, Lasse Reichstein Nielsen wrote:
> > Committed patchset #9 manually as r34967 (presubmit successful).
> 
> This CL, if I understand it, is to support running more tests on d8.
> However, it adds 1.2k to swarm.

That's a lot for relatively few lines added in async_patch.
Maybe those lines can be refactored, or reuse some code from the other
alternatives.

> Is it possible to refactor it to push more of the tax into preambles/d8.js ?

I hope so. If not this part, then something else.