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

cluster: use rest param & Reflect.apply #17655

Closed
wants to merge 1 commit into from
Closed

cluster: use rest param & Reflect.apply #17655

wants to merge 1 commit into from

Conversation

mithunsasidharan
Copy link
Contributor

@mithunsasidharan mithunsasidharan commented Dec 13, 2017

Used Reflect.apply for performance gain citing similar change in fs.js

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

cluster

@nodejs-github-bot nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Dec 13, 2017
@mithunsasidharan
Copy link
Contributor Author

@apapirovski : Your thoughts on whether this helps ? This is citing previous PR #17486 . If you feel this helps, I'll open it for review.

@apapirovski
Copy link
Member

apapirovski commented Dec 13, 2017

I would recommend checking whether the cb in this case is user-provided or just something internal within Node.js. Mostly we're switching to Reflect.apply because it's safer when dealing with user-provided callbacks, rather than because it boosts perf by like 1-2% in some edge cases.

@mithunsasidharan
Copy link
Contributor Author

@apapirovski : It seems to me that it is internal. But does it matter really with regards to perf gain ? In all cases shouldn't it help with that 1-2 % as mentioned ? Thanks.

@apapirovski
Copy link
Member

@mithunsasidharan You have to consider whether this is a hot path code or not. Otherwise the 1% faster apply call might convert to 0.00001% real world impact or less. I would say it's probably not worth it.

@mithunsasidharan
Copy link
Contributor Author

mithunsasidharan commented Dec 13, 2017

@apapirovski : Sure.. let's park it for now... I'll probably check in detail over weekend and see if it really helps as you mentioned. I'll get back to some coverage for now ! Meanwhile I found this went unnoticed and thought I could raise a PR for it ? nodejs/code-and-learn#72 (comment) Do you suggest we should probably create an issue for this and leave it for some starters ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants