lib: reduce process.binding() calls #1367
Conversation
This commit reduces the amount of times process.binding() is called by better lazy loading the values and always using a variable to hold the value instead of loading it each time. This is important because, in my benchmarks, running process.binding() is about 172 times slower as referencing a variable with the same value.
| @@ -417,12 +417,12 @@ function setupChannel(target, channel) { | |||
| message.type = 'net.Socket'; | |||
| } else if (handle instanceof net.Server) { | |||
| message.type = 'net.Server'; | |||
| } else if (handle instanceof process.binding('tcp_wrap').TCP || | |||
| handle instanceof process.binding('pipe_wrap').Pipe) { | |||
| } else if (handle instanceof handleWraps.TCP || | |||
There was a problem hiding this comment.
Where are handleWraps properties being set?
There was a problem hiding this comment.
here and the function below, it's a Getter
|
Can't we just do all of these |
|
Yeah, I think it would probably be better to just do them at startup. |
Generally better. Hoists all the process.binding calls and removes some of the now-pointless wrapper functions
|
Good call, updated to hoist all the things. PTAL @mscdex @Fishrock123 |
|
LGTM if it passes all tests. |
|
Passes tests on my mac, is it worth it to run a CI? (I don't have access) |
|
It might be worth it, in case loading a binding (e.g. tty) has some side effects that I'm not aware of. I don't have access to CI either, so I can't help there :-) |
|
It doesn't seem to have anything better to do; build here: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/460/ |
| @@ -3,6 +3,7 @@ | |||
| const net = require('net'); | |||
| const url = require('url'); | |||
| const util = require('util'); | |||
| const binding = process.binding('crypto'); | |||
There was a problem hiding this comment.
I think the different naming is possibly to differentiate it from the result of require('crypto'). Plus it's the only binding being loaded in the file.
|
Was there a reason these were lazy loaded before? How is the startup time affected? |
|
@petkaantonov most probably because of speed, but the |
by what sort of factor? it's important to optimise for quick startup, we shouldn't lose sight of that as an important metric of speed |
|
@rvagg from this quick benchmark I wrote,
|
|
CI for good measure: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/466/ |
|
On 466, the windows 2008 failures seem to be timeouts unrelated to this PR, otherwise it looks happy. I'm thinking I'll merge this in tomorrow (w/o objections) unless someone wants to throw in another sign-off. |
| } else if (handle instanceof process.binding('tcp_wrap').TCP || | ||
| handle instanceof process.binding('pipe_wrap').Pipe) { | ||
| } else if (handle instanceof TCP || | ||
| handle instanceof Pipe) { |
|
LGTM with nits. |
This commit better handles calls to process.binding() in lib/ by no longer lazy loading the bindings (the load times themselves are rather miniscule compared to the load time of V8) and never reloading the bindings (which is 172 times slower than referencing a variable with the same value). PR-URL: #1367 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
Thanks, this has been merged in |
This commit reduces the amount of times
process.binding()is called bybetter lazy loading the values and always using a variable to hold the
value instead of loading it each time.
This is important because, in my benchmarks, running process.binding()
is about 172 times slower as referencing a variable with the same
value.