Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

dns: allow v8 to optimize lookup() #8942

Closed
wants to merge 1 commit into from
Closed

Conversation

mscdex
Copy link

@mscdex mscdex commented Dec 25, 2014

Reassigning the values of parameter variables causes
v8 to disable function optimization.

exports.lookup = function(domain, family, callback) {
exports.lookup = function(domain, family_, callback_) {
var family = family_,
callback = callback;
Copy link

Choose a reason for hiding this comment

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

Should this be callback = callback_?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, sorry.

@cjihrig
Copy link

cjihrig commented Dec 25, 2014

I thought v8 could still optimize writes to named function arguments, but couldn't handle writes to arguments (need to test though).

@mscdex
Copy link
Author

mscdex commented Dec 25, 2014

It can't optimize any function that contains any usage of arguments and also mutates parameter variable values. This is mentioned in bluebird's "Optimization killers" and I also saw it while checking for deoptimizations in my own code with node v0.10.

@Fishrock123
Copy link
Member

I thought v8 could still optimize writes to named function arguments, but couldn't handle writes to arguments (need to test though).

I was also under this impression. I know writing to arguments is bad, but I always do like fn(param) { param = param || 'thing' }.

@Fishrock123
Copy link
Member

This is mentioned in bluebird's "Optimization killers"

Hmm, only says when you also use arguments in the same function. Interesting. Weird.

@cjihrig
Copy link

cjihrig commented Dec 25, 2014

Ah, the problem is that is also reads arguments. I think you can get around that just by enabling strict mode.

Using `arguments` and reassigning parameter variable
values causes v8 to disable function optimization.
@jasnell
Copy link
Member

jasnell commented Aug 27, 2015

@mscdex ... is this still something you think needs to be done in v0.10?

@mscdex
Copy link
Author

mscdex commented Aug 27, 2015

@jasnell Well, it's an optimization. If anything it might make lookups a tad faster. If it doesn't apply cleanly to v0.10 anymore I guess I wouldn't worry about it.

@jasnell
Copy link
Member

jasnell commented Aug 27, 2015

So long as CI passes, LGTM

jasnell pushed a commit that referenced this pull request Aug 27, 2015
Using `arguments` and reassigning parameter variable
values causes v8 to disable function optimization.

PR-URL: #8942
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Aug 27, 2015

Landed in 6502160

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants