Pass args to process.nextTick() #1077

trevnorris · 2015-03-05T22:52:37Z

Now allow process.nextTick(callback[, ... vargs])

cjihrig · 2015-03-05T23:11:55Z

lib/_stream_writable.js

@@ -339,6 +337,8 @@ function onwrite(stream, er) {
    }

    if (sync) {
+      // TODO(trevnorris): Yank this, but first need to figure out why


Any chance of figuring this out before merging?

I will. Just don't have the brain cells at the moment to figure out why process.nextTick() returns undefined in the mentioned test.

I just tested this and all tests (including test-repl-timeout-throw) still pass after changing this additional instance of process.nextTick() to process.nextTick(afterWrite, stream, state, finished, cb);

Thanks. I'll make the change to the PR.

rvagg · 2015-03-05T23:15:26Z

this is a tiny bit gross and a quite a leaky abstraction, I'm not really a fan of exposing ugly APIs just because it's the fastest way--fine if it's an internal API for the sake of cleaning up and speeding up (does it really do either of those?) but now we have to expose this to users.

process.nextTick(cb[, arg1[, arg2... ] ]) would be the obvious API choice because of the consistency and my vote would be that if we can't do that without taking a performance hit then we shouldn't do anything.

trevnorris · 2015-03-05T23:22:01Z

@rvagg leaky? and it does both clean up and speed up (removing the need for additional function closures, flatten function declarations and no need to create an actual array). I played with the other API decision, but it causes far too many DEOPTs in nextTick() to be useful. And I disagree we shouldn't do anything. Internals are a CF of function declarations that prevent further optimizations.

rvagg · 2015-03-05T23:25:45Z

leaky abstraction in the sense that your abstraction is saying too much about the implementation -- you're declaring to the world that you had to make compromises on your API to get other outcomes (performance), there has to be a tradeoff between pure perf and the best internal implementation and the API we expose to users and I'm here representing the API and this is that tradeoff discussion

piscisaureus · 2015-03-05T23:53:41Z

I agree with @rvagg. This adds API that may seem nice and fast now but we have to support it forever. It would be more helpful if the setArgs api was strictly internal.

cjihrig · 2015-03-05T23:57:20Z

@piscisaureus as in _setArgs()?

tellnes · 2015-03-06T01:32:28Z

I also agree with @rvagg on this. If we do need an ugly API, then let us try to find a way to not expose it.

vkurchatkin · 2015-03-06T07:16:18Z

+1 on making this internal. See discussion in #953

medikoo · 2015-03-06T07:57:21Z

This is quite dirty design, totally not common to similar API's, that people are familiar with.
-1 on having this public, whatever on internal.

trevnorris · 2015-03-06T16:08:01Z

@medikoo I don't appreciate "dirty design". Yes it's uncommon, but in terms of code complexity and performance it's the cleanest.

setArgs() is now _setArgs().

vkurchatkin · 2015-03-06T18:37:07Z

setArgs() is now _setArgs().

sigh one more "private" thing that people will use

trevnorris · 2015-03-06T20:33:26Z

@vkurchatkin I figured the fact that using process.nextTick() is already frowned upon, and the fact that this API isn't documented was enough. Guess a simple _ gives people a feeling of security.

rvagg · 2015-03-06T20:33:41Z

sigh one more "private" thing that people will use

I echo your sigh here, could we make use of Symbol here maybe or should we just get that internal modules thing sorted out?

trevnorris · 2015-03-06T20:34:52Z

@rvagg unless we're willing to either 1) remove process.nextTick() completely, or 2) have two implementations of nexTick() (one internal, the other user facing) neither of those solutions will work.

vkurchatkin · 2015-03-06T21:26:42Z

should we just get that internal modules thing sorted out

we can have this without internal modules

@trevnorris I'm thinking about 2: user facing one would be just a wrapper of internal one

chrisdickinson · 2015-03-10T18:47:46Z

lib/_stream_readable.js

-      process.nextTick(function() {
-        emitReadable_(stream);
-      });
+      process.nextTick(emitReadable_)._setArgs(stream);


This may prove problematic for porting it to readable-stream, since it's using an API that browserify (almost certainly) does not support.

chrisdickinson · 2015-03-10T18:59:12Z

If we end up going this route, I'm in favor of going the internal module + private symbol approach for solving this so we don't expose the setArg API to the world. I really don't want a "internal only" nextTick API.

(This is also a fairly precarious change for readable-stream, no matter which way the flow of code goes. Either way, it may not have access to .setArg.)

vkurchatkin · 2015-03-10T20:52:24Z

@chrisdickinson I propose injecting private nextTick into internal modules with this new functionality. No symbols are required

chrisdickinson · 2015-03-10T20:55:38Z

@vkurchatkin Then we have two nextTick's, one private and one public. I'd rather just make this one sub-method private.

vkurchatkin · 2015-03-10T21:02:11Z

@chrisdickinson What I mean is something like this: process.nextTick = function(cb) { nextTick(cb); }. The same function, just return undefined

sam-github · 2015-03-11T00:11:58Z

@trevnorris can you comment on why this API is faster than

process.nextTick(fn, [ctx])

?

I assume it was because you don't want to slice fn off the start of arguments before applying the remaing args to fn, but if you limit to one arg, you don't have to.

chrisdickinson · 2015-03-11T00:16:25Z

@vkurchatkin How does that private nextTick get shared with _stream_readable.js?

vkurchatkin · 2015-03-11T00:22:14Z

it is passed as an argument to module wrapper. Not a good idea for _stream_readable.js though as it is supposed to be the same as in readable-stream, so only public APIs.

sam-github · 2015-03-11T00:39:28Z

Oh, and if nextTick is worth making better for use in iojs, its worth making it better for everybody, IMHO.

trevnorris · 2015-03-11T19:17:26Z

@sam-github It makes the call to nextTick() polymorphic. And only allowing a single argument isn't enough in many cases. So it could possibly make the call megamorphic. Thus preventing nextTick() from being inlined.

trevnorris · 2015-03-11T19:20:05Z

I won't accept nextTick(callback[, ... vargs]) because of how it'll affect performance. And readable-stream only uses public API. So either we expose this or nothing goes in.

chrisdickinson · 2015-03-13T00:34:14Z

Just thinking through this out loud, with regards to readable streams: even with an exposed setArg API, wouldn't this preclude us from importing these changes into the streams3 readable-stream branch?

sam-github · 2015-03-17T22:25:23Z

@trevnorris thanks, I get it. It is unfortunately ugly... but that might limit its use to just performance-critical code... which would be OK.

Is megamorphic even a word? :-)

petkaantonov · 2015-03-27T21:07:10Z

This could be also merged right now as an unobservable change (with the doc changes reverted) and make the public API "release" later in a semver-minor.

trevnorris · 2015-03-27T21:37:25Z

Thanks @petkaantonov. That would be my preference.

Fishrock123 · 2015-04-08T14:02:21Z

@trevnorris done any benchmarks yet?

I see that was the resolution as of the tc-meeting it was discussed in.

trevnorris · 2015-04-08T17:24:47Z

@Fishrock123 some. Initial results showed improvements just above the margin of error. Part of the gain also comes from easier performance debugging, since functions won't DEOPT from being scoped. It's possible to create a benchmark that shows significant gains, but this is definitely a micro optimization.

trevnorris · 2015-04-15T21:34:07Z

@iojs/tc Was this supposed to have landed before the 1.7 release?

cjihrig · 2015-04-15T22:25:00Z

LGTM. Starting CI to verify.

cjihrig · 2015-04-15T22:26:50Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/515/

PR-URL: #1077 Reviewed-by: Colin Ihrig <[email protected]>

trevnorris · 2015-04-15T23:07:50Z

Thanks. Landed in 10e31ba.

jbergstroem · 2015-04-16T01:05:37Z

Possibly stupid question, but seeing this is server-minor - is 1.8.0 our next release?

rvagg · 2015-04-16T01:11:17Z

@jbergstroem yes, either that or 2.0.0 if we get the https://github.com/iojs/io.js/milestones/2.0.0 changes sorted out

trevnorris · 2015-04-16T01:31:21Z

@jbergstroem this should have been merged before 1.7 but wasn't.

jbergstroem · 2015-04-16T01:33:01Z

Ok. I was pretty keen on getting 1.7.2 within a week or so with a fix to a shared build. Guessing 2.0 might make that easier since we'd branch off to master/1.x/2.x?

Notable Changes: * build: Support for building io.js as a static library (Marat Abdullin) #1341 * deps: upgrade openssl to 1.0.2a (Shigeki Ohtsu) #1389 * npm: Upgrade npm to 2.8.3. (Forrest L Norvell) #1448 * src: allow multiple arguments to be passed to process.nextTick (Trevor Norris) #1077 * module: interaction of require('.') with NODE_PATH has been restored and deprecated. This functionality will be removed at a later point. (Roman Reiss) #1363

Notable Changes: * build: Support for building io.js as a static library (Marat Abdullin) #1341 * deps: upgrade openssl to 1.0.2a (Shigeki Ohtsu) #1389 * npm: Upgrade npm to 2.8.3. (Forrest L Norvell) #1448 * src: allow multiple arguments to be passed to process.nextTick (Trevor Norris) #1077 * module: the interaction of require('.') with NODE_PATH has been restored and deprecated. This functionality will be removed at a later point. (Roman Reiss) #1363

19h · 2015-04-18T21:12:34Z

🎉 🎉 Awesome! 🎉 🎉

Notable Changes: * build: revert vcbuild.bat changes * changes inherited from v1.8.0: * build: Support for building io.js as a static library (Marat Abdullin) #1341 * npm: Upgrade npm to 2.8.3. (Forrest L Norvell) #1448 * deps: upgrade openssl to 1.0.2a (Shigeki Ohtsu) #1389 * src: allow multiple arguments to be passed to process.nextTick (Trevor Norris) #1077 * module: the interaction of require('.') with NODE_PATH has been restored and deprecated. This functionality will be removed at a later point. (Roman Reiss) #1363

igl · 2015-04-23T07:54:58Z

Since nobody mentioned it: Isn't callback.bind(ctx, ...args) the same thing? Even more extensive with the possibility of setting a context.

petkaantonov · 2015-04-23T08:03:03Z

@igl Yes, well only that, it's like 100000x slower.

trevnorris force-pushed the pass-args-to-nextTick branch from 0d1d754 to 5e34bae Compare March 5, 2015 23:09

cjihrig reviewed Mar 5, 2015
View reviewed changes

trevnorris force-pushed the pass-args-to-nextTick branch from 5e34bae to 8828d49 Compare March 6, 2015 16:06

chrisdickinson reviewed Mar 10, 2015
View reviewed changes

trevnorris added a commit that referenced this pull request Apr 15, 2015

node: allow multiple arguments passed to nextTick

10e31ba

PR-URL: #1077 Reviewed-by: Colin Ihrig <[email protected]>

trevnorris closed this Apr 15, 2015

chrisdickinson mentioned this pull request Apr 16, 2015

Release proposal: 1.8.0 #1436

Closed

This was referenced Apr 23, 2015

pass arguments to nextTick defunctzombie/node-process#42

Closed

let process.nextTick accept arguments defunctzombie/node-process#43

Merged

aearly mentioned this pull request May 7, 2015

callbackNextTick method added to API caolan/async#750

Closed

trevnorris deleted the pass-args-to-nextTick branch October 23, 2015 21:46

quentin-ol mentioned this pull request Nov 10, 2015

process.nextTick and io.js versions up to 1.7.1 mqttjs/mqtt-packet#7

Closed

Pass args to process.nextTick() #1077

Pass args to process.nextTick() #1077

Conversation

trevnorris commented Mar 5, 2015

cjihrig Mar 5, 2015

Choose a reason for hiding this comment

trevnorris Mar 5, 2015

Choose a reason for hiding this comment

mscdex Mar 25, 2015

Choose a reason for hiding this comment

trevnorris Mar 25, 2015

Choose a reason for hiding this comment

rvagg commented Mar 5, 2015

trevnorris commented Mar 5, 2015

rvagg commented Mar 5, 2015

piscisaureus commented Mar 5, 2015

cjihrig commented Mar 5, 2015

tellnes commented Mar 6, 2015

vkurchatkin commented Mar 6, 2015

medikoo commented Mar 6, 2015

trevnorris commented Mar 6, 2015

vkurchatkin commented Mar 6, 2015

trevnorris commented Mar 6, 2015

rvagg commented Mar 6, 2015

trevnorris commented Mar 6, 2015

vkurchatkin commented Mar 6, 2015

chrisdickinson Mar 10, 2015

Choose a reason for hiding this comment

chrisdickinson commented Mar 10, 2015

vkurchatkin commented Mar 10, 2015

chrisdickinson commented Mar 10, 2015

vkurchatkin commented Mar 10, 2015

sam-github commented Mar 11, 2015

chrisdickinson commented Mar 11, 2015

vkurchatkin commented Mar 11, 2015

sam-github commented Mar 11, 2015

trevnorris commented Mar 11, 2015

trevnorris commented Mar 11, 2015

chrisdickinson commented Mar 13, 2015

sam-github commented Mar 17, 2015

petkaantonov commented Mar 27, 2015

trevnorris commented Mar 27, 2015

Fishrock123 commented Apr 8, 2015

trevnorris commented Apr 8, 2015

trevnorris commented Apr 15, 2015

cjihrig commented Apr 15, 2015

cjihrig commented Apr 15, 2015

trevnorris commented Apr 15, 2015

jbergstroem commented Apr 16, 2015

rvagg commented Apr 16, 2015

trevnorris commented Apr 16, 2015

jbergstroem commented Apr 16, 2015

19h commented Apr 18, 2015

igl commented Apr 23, 2015

petkaantonov commented Apr 23, 2015