Let's overhaul `nextTick` #59

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
4 participants
@domenic
Collaborator

domenic commented Apr 21, 2012

Let's get rid of the event-queue dependency and use setImmediate if available. This would close #50 and #44.

@kriskowal

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 23, 2012

Owner

I fully endorse the removal of the Narwhal code. It’s removal implies that we can remove the bootstrapping magic for "require" or "serverSideRequire" in the header.

I am waffling on "setImmediate". We could probably lose that too. But if you want it, we can keep it.

It strikes me that the queue logic might be simplified.

I think we should land this pull request at least with the simplified bootstrapping. Consider my other comments optional and feel free to merge these yourself, @domenic.

Owner

kriskowal commented Apr 23, 2012

I fully endorse the removal of the Narwhal code. It’s removal implies that we can remove the bootstrapping magic for "require" or "serverSideRequire" in the header.

I am waffling on "setImmediate". We could probably lose that too. But if you want it, we can keep it.

It strikes me that the queue logic might be simplified.

I think we should land this pull request at least with the simplified bootstrapping. Consider my other comments optional and feel free to merge these yourself, @domenic.

@Yaffle

This comment has been minimized.

Show comment Hide comment
@Yaffle

Yaffle Apr 24, 2012

Is head.task still reference to the task function after task is executed ?

Yaffle commented on q.js in 0b23c1e Apr 24, 2012

Is head.task still reference to the task function after task is executed ?

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 24, 2012

Owner

Yes, it seems so.

Owner

kriskowal replied Apr 24, 2012

Yes, it seems so.

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 24, 2012

Collaborator

I'm not following. Is that bad? Should we fix it?

Collaborator

domenic replied Apr 24, 2012

I'm not following. Is that bad? Should we fix it?

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 24, 2012

Owner

It might be possible to express these four lines in as few as two.

Owner

kriskowal replied Apr 24, 2012

It might be possible to express these four lines in as few as two.

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

Got it. head.next.task(); head = head.next; right?

Collaborator

domenic replied Apr 25, 2012

Got it. head.next.task(); head = head.next; right?

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

That ought to do the trick.

Owner

kriskowal replied Apr 25, 2012

That ought to do the trick.

This comment has been minimized.

Show comment Hide comment
@Yaffle

Yaffle Apr 25, 2012

head = head.next;
var task = head.task;
head.task = null;
task();
head = head.next;
var task = head.task;
head.task = null;
task();

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

Looks like @Yaffle's right, we need 4 lines to avoid holding on to the reference. Agree, @kriskowal?

Collaborator

domenic replied Apr 25, 2012

Looks like @Yaffle's right, we need 4 lines to avoid holding on to the reference. Agree, @kriskowal?

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

I don’t think that’s necessary. The first line drops the previous head, so it should not be accessible through exploration from any GC root.

Owner

kriskowal replied Apr 25, 2012

I don’t think that’s necessary. The first line drops the previous head, so it should not be accessible through exploration from any GC root.

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

Right, but the current head and head.task is still there. There will be at all times a single task still being held on to, no?

Collaborator

domenic replied Apr 25, 2012

Right, but the current head and head.task is still there. There will be at all times a single task still being held on to, no?

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

If there's anything to tweak in the above commits, let me know. (Changelog wording perhaps?) Otherwise I'll squash all of them into two ("Remove Narwhal support" and "Use setImmediate if available") and push.

Collaborator

domenic commented Apr 25, 2012

If there's anything to tweak in the above commits, let me know. (Changelog wording perhaps?) Otherwise I'll squash all of them into two ("Remove Narwhal support" and "Use setImmediate if available") and push.

@@ -23,50 +23,49 @@
// RequireJS
if (typeof define === "function") {
- define(definition);
+ define(["exports"], definition);

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

I presume that this is necessary and has been tested with RequireJS or maybe even a derivative of RequireJS?

@kriskowal

kriskowal Apr 25, 2012

Owner

I presume that this is necessary and has been tested with RequireJS or maybe even a derivative of RequireJS?

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

Oh, it’s because of the omission of require

@kriskowal

kriskowal Apr 25, 2012

Owner

Oh, it’s because of the omission of require

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

Right, exactly. It's in the AMD spec so it should be good across AMD loaders.

@domenic

domenic Apr 25, 2012

Collaborator

Right, exactly. It's in the AMD spec so it should be good across AMD loaders.

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

Sounds good to me. I believe @jrburke has a vested interest in this continuing to function properly, so I’m name-dropping him here :P

@kriskowal

kriskowal Apr 25, 2012

Owner

Sounds good to me. I believe @jrburke has a vested interest in this continuing to function properly, so I’m name-dropping him here :P

This comment has been minimized.

Show comment Hide comment
@jrburke

jrburke Apr 25, 2012

Collaborator

Looks good to me, and yes, I definitely want to still use q.

@jrburke

jrburke Apr 25, 2012

Collaborator

Looks good to me, and yes, I definitely want to still use q.

@kriskowal

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

Looks much cleaner. Thanks.

Owner

kriskowal commented Apr 25, 2012

Looks much cleaner. Thanks.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

@Yaffle, @kriskowal what do you think of 3175d1c.

Collaborator

domenic commented Apr 25, 2012

@Yaffle, @kriskowal what do you think of 3175d1c.

@kriskowal

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

Would an exception thrown here halt the processing of subsequent tasks?

Owner

kriskowal commented on q.js in 3175d1c Apr 25, 2012

Would an exception thrown here halt the processing of subsequent tasks?

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

Indeed it would; that's bad. I wonder if the deoptimization from try/catch would overwhelm the benefit of batching, at least in the MessageChannel case. In the setTimeout(, 0) case I'm sure this would still come out ahead.

But hmm, then you have to re-throw the errors in the next tick, so ugh, this is getting worse and worse.

Collaborator

domenic replied Apr 25, 2012

Indeed it would; that's bad. I wonder if the deoptimization from try/catch would overwhelm the benefit of batching, at least in the MessageChannel case. In the setTimeout(, 0) case I'm sure this would still come out ahead.

But hmm, then you have to re-throw the errors in the next tick, so ugh, this is getting worse and worse.

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

Good point about MessageChannel. Batching might not benefit that approach. Of course everything in a Q callback is already suffering the try/catch penalty once. I wonder whether they compound. In any case, perhaps it is not worth pursuing after all.

Owner

kriskowal replied Apr 25, 2012

Good point about MessageChannel. Batching might not benefit that approach. Of course everything in a Q callback is already suffering the try/catch penalty once. I wonder whether they compound. In any case, perhaps it is not worth pursuing after all.

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

That's true, and they might not compound. (In my uninformed mental model, it just flips the "be exception-aware" bit.) I think for setTimeout it's definitely worth pursuing. It does make the existing nextTick(function () { throw error; }) code a bit comical, though.

Collaborator

domenic replied Apr 25, 2012

That's true, and they might not compound. (In my uninformed mental model, it just flips the "be exception-aware" bit.) I think for setTimeout it's definitely worth pursuing. It does make the existing nextTick(function () { throw error; }) code a bit comical, though.

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

Rethrowing exceptions in future turns to avoid being caught…is already a bit of dark humor. To be caught and rethrown in that future turn just makes it darker and more humorous.

Owner

kriskowal replied Apr 25, 2012

Rethrowing exceptions in future turns to avoid being caught…is already a bit of dark humor. To be caught and rethrown in that future turn just makes it darker and more humorous.

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

Hah, well put. I'll go for it. The next big thing I want to tackle is #57 anyway, so that would hide all the dark humor far, far from the light of day.

Collaborator

domenic replied Apr 25, 2012

Hah, well put. I'll go for it. The next big thing I want to tackle is #57 anyway, so that would hide all the dark humor far, far from the light of day.

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

Bah. Now the following code is out of order:

nextTick2(console.log.bind(console, "1"));
nextTick2(function () { throw new RangeError("ahsdhfkdlf"); });
nextTick2(console.log.bind(console, "3"));

With the current implementation in master, you get 1, RangeError, 2 in the console. With a version of this that involves catching and rethrowing, you get 1, 3, error.

Think it's time to abandon this approach :(

Collaborator

domenic replied Apr 25, 2012

Bah. Now the following code is out of order:

nextTick2(console.log.bind(console, "1"));
nextTick2(function () { throw new RangeError("ahsdhfkdlf"); });
nextTick2(console.log.bind(console, "3"));

With the current implementation in master, you get 1, RangeError, 2 in the console. With a version of this that involves catching and rethrowing, you get 1, 3, error.

Think it's time to abandon this approach :(

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

Yeah, I am far more anxious to see #57. It is a game changer.

Owner

kriskowal replied Apr 25, 2012

Yeah, I am far more anxious to see #57. It is a game changer.

@kriskowal

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Apr 25, 2012

Owner

I am curious whether an array or a linked list would run faster here. My theory is that array.shift() is O(array.length) and that a linked list would get JITed down to structs.

Owner

kriskowal commented on q.js in 3175d1c Apr 25, 2012

I am curious whether an array or a linked list would run faster here. My theory is that array.shift() is O(array.length) and that a linked list would get JITed down to structs.

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

Good point, hmm. Seems very JITter dependent. Wonder if this is decidable with jsperf.

Collaborator

domenic replied Apr 25, 2012

Good point, hmm. Seems very JITter dependent. Wonder if this is decidable with jsperf.

This comment has been minimized.

Show comment Hide comment
@Yaffle

Yaffle Apr 25, 2012

for Chrome shift+push works fast,
but it is implementation dependent, while linked list is fast everywhere

for Chrome shift+push works fast,
but it is implementation dependent, while linked list is fast everywhere

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 25, 2012

Collaborator

Closed in favor of squashed commits.

Collaborator

domenic commented Apr 25, 2012

Closed in favor of squashed commits.

@domenic domenic closed this Apr 25, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment