Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.