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.

Sign up for GitHub

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

Jump to bottom

benchmark: removed unused arguments from callbacks #14919

Closed
wants to merge 1 commit into from
Closed

benchmark: removed unused arguments from callbacks #14919

wants to merge 1 commit into from

Conversation

abhishek-raj
Copy link
Contributor

Removed unused arguments 'buf' and 'rinfo' from the callbacks.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmark

@abhishek-raj
benchmark: removed unused arguments from callbacks
e086861 
Removed unused arguments 'buf' and 'rinfo' from the callbacks.
@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. labels Aug 18, 2017
@mscdex
Copy link
Contributor

mscdex commented Aug 18, 2017 •
edited
Loading

FWIW (in general) the reason this was done was because at least some time ago, V8 would perform optimizations when the number of parameters matched the number of arguments being passed. I do not know if this is the case any longer though.

@vsemozhetbyt
Copy link
Contributor

cc @nodejs/v8

@Trott
Copy link
Member

Trott commented Aug 18, 2017 •
edited
Loading

Quick side-by-side runs seem to suggest that removing the unused arguments seems to not significantly affect performance of these benchmarks. But a more thorough check or informed opinion would be welcome.

Trott
Trott approved these changes Aug 18, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

tniessen
tniessen approved these changes Aug 18, 2017
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Change LGTM, performance should be considered by @nodejs/v8.

@tniessen
Copy link
Member

Pre-land CI: https://ci.nodejs.org/job/node-test-pull-request/9771/

jasnell pushed a commit that referenced this pull request Aug 21, 2017
@abhishek-raj @jasnell
benchmark: removed unused arguments from callbacks
36817f7 
Removed unused arguments 'buf' and 'rinfo' from the callbacks.

PR-URL: #14919
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 21, 2017

Landed in 36817f7

@jasnell jasnell closed this Aug 21, 2017
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@abhishek-raj @MylesBorins
benchmark: removed unused arguments from callbacks
45aa860 
Removed unused arguments 'buf' and 'rinfo' from the callbacks.

PR-URL: #14919
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@abhishek-raj @MylesBorins
benchmark: removed unused arguments from callbacks
7f1ea7c 
Removed unused arguments 'buf' and 'rinfo' from the callbacks.

PR-URL: #14919
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@tniessen tniessen tniessen approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
benchmark Issues and PRs related to the benchmark subsystem. dgram Issues and PRs related to the dgram subsystem / UDP.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@abhishek-raj @mscdex @vsemozhetbyt @Trott @tniessen @jasnell @lpinca @BridgeAR @MylesBorins @nodejs-github-bot