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

lib: reduce process.binding() calls #1367

Closed

Conversation

brendanashworth
Copy link
Contributor

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.

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

Choose a reason for hiding this comment

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

Where are handleWraps properties being set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here and the function below, it's a Getter

@mscdex
Copy link
Contributor

mscdex commented Apr 8, 2015

Can't we just do all of these process.binding() calls on startup, assigning them to a const variable, instead of having an if (xxx === undefined) .... everywhere?

@Fishrock123
Copy link
Contributor

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

Good call, updated to hoist all the things. PTAL @mscdex @Fishrock123

@mscdex
Copy link
Contributor

mscdex commented Apr 8, 2015

LGTM if it passes all tests.

@brendanashworth
Copy link
Contributor Author

Passes tests on my mac, is it worth it to run a CI? (I don't have access)

@mscdex
Copy link
Contributor

mscdex commented Apr 8, 2015

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

@jbergstroem
Copy link
Member

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');
Copy link
Contributor

Choose a reason for hiding this comment

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

crypto?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 stole it from the other modules - I originally had it as cryptoBinding.

@petkaantonov
Copy link
Contributor

Was there a reason these were lazy loaded before? How is the startup time affected?

@brendanashworth
Copy link
Contributor Author

@petkaantonov most probably because of speed, but the process.binding() call speed alone is dwarfed by v8 module load time anyways so it probably isn't worth optimizing for.

@rvagg
Copy link
Member

rvagg commented Apr 8, 2015

dwafed

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

@brendanashworth
Copy link
Contributor Author

@rvagg from this quick benchmark I wrote, require('child_process'); vs all the process.bindings inside child_process speed is about* a factor of 1000/1.

*was too lazy to figure out how to clear the module cache

@Fishrock123
Copy link
Contributor

@brendanashworth
Copy link
Contributor Author

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) {
Copy link
Member

Choose a reason for hiding this comment

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

Fits on a single line now.

@bnoordhuis
Copy link
Member

LGTM with nits.

@brendanashworth brendanashworth self-assigned this Apr 9, 2015
brendanashworth added a commit that referenced this pull request Apr 9, 2015
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>
@brendanashworth
Copy link
Contributor Author

Thanks, this has been merged in 1219e74.

@brendanashworth brendanashworth removed their assignment Apr 9, 2015
@rvagg rvagg mentioned this pull request Apr 11, 2015
@brendanashworth brendanashworth deleted the reduce-process-binding branch April 18, 2015 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants