Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass args to process.nextTick() #1077

Closed
wants to merge 1 commit into from

Conversation

trevnorris
Copy link
Contributor

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

R=@bnoordhuis

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance of figuring this out before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rvagg
Copy link
Member

rvagg commented Mar 5, 2015

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
Copy link
Contributor Author

@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
Copy link
Member

rvagg commented Mar 5, 2015

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
Copy link
Contributor

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
Copy link
Contributor

cjihrig commented Mar 5, 2015

@piscisaureus as in _setArgs()?

@tellnes
Copy link
Contributor

tellnes commented Mar 6, 2015

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
Copy link
Contributor

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

@medikoo
Copy link

medikoo commented Mar 6, 2015

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
Copy link
Contributor Author

@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
Copy link
Contributor

setArgs() is now _setArgs().

sigh one more "private" thing that people will use

@trevnorris
Copy link
Contributor Author

@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
Copy link
Member

rvagg commented Mar 6, 2015

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

process.nextTick(function() {
emitReadable_(stream);
});
process.nextTick(emitReadable_)._setArgs(stream);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@chrisdickinson
Copy link
Contributor

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
Copy link
Contributor

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

@chrisdickinson
Copy link
Contributor

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

@vkurchatkin
Copy link
Contributor

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

@sam-github
Copy link
Contributor

@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
Copy link
Contributor

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

@vkurchatkin
Copy link
Contributor

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
Copy link
Contributor

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

@trevnorris
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor Author

Thanks @petkaantonov. That would be my preference.

@Fishrock123
Copy link
Contributor

@trevnorris done any benchmarks yet?

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

@trevnorris
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@cjihrig
Copy link
Contributor

cjihrig commented Apr 15, 2015

LGTM. Starting CI to verify.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 15, 2015

trevnorris added a commit that referenced this pull request Apr 15, 2015
@trevnorris
Copy link
Contributor Author

Thanks. Landed in 10e31ba.

@trevnorris trevnorris closed this Apr 15, 2015
@jbergstroem
Copy link
Member

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

@rvagg
Copy link
Member

rvagg commented Apr 16, 2015

@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
Copy link
Contributor Author

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

@jbergstroem
Copy link
Member

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?

chrisdickinson added a commit that referenced this pull request Apr 17, 2015
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
chrisdickinson added a commit that referenced this pull request Apr 17, 2015
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
Copy link
Contributor

19h commented Apr 18, 2015

🎉 🎉 Awesome! 🎉 🎉

chrisdickinson added a commit that referenced this pull request Apr 20, 2015
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
Copy link

igl commented Apr 23, 2015

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

@petkaantonov
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.