Skip to content

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
Contributor

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
Contributor

@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