Skip to content

lib: Enhancing Performance and Readability with Switch Statements 🚀 #51281

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

Closed

Conversation

sanjaiyan-dev
Copy link
Contributor

Hi, this PR will result in a very minimal performance improvement by changing from multiple if conditions to a switch statement. Since a switch statement utilizes a branch table (correct me if I'm wrong), the gain in performance will be marginal, but it enhances code readability.

I've attached a picture of the JavaScript benchmark I ran, and there's a YouTube video demonstrating how a switch statement can lead to performance gains. The video showcases how the C compiler optimizes switch statements, and if I'm correct, this optimization principle should also apply to the V8 engine, which JIT compiles JavaScript to assembly language: YouTube link.

IMG_20231225_020607
IMG_20231225_020629

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Dec 24, 2023
@juanarbol
Copy link
Member

Hi! Thanks for this PR, could you please add benchmarks for it? Looking good so far

Please consider reading the contributing guidelines https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#the-process-of-making-changes

@sanjaiyan-dev
Copy link
Contributor Author

sanjaiyan-dev commented Dec 25, 2023

Hi! Thanks for this PR, could you please add benchmarks for it? Looking good so far

https://jsben.ch/5w6eT

@sanjaiyan-dev

This comment was marked as off-topic.

@aduh95 aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label Dec 25, 2023
@aduh95
Copy link
Contributor

aduh95 commented Dec 25, 2023

Hi! Thanks for this PR, could you please add benchmarks for it? Looking good so far

https://jsben.ch/5w6eT

This does not mean anything, you're not testing Node.js, and you're not even testing the same code.

@sanjaiyan-dev
Copy link
Contributor Author

Hi! Thanks for this PR, could you please add benchmarks for it? Looking good so far

https://jsben.ch/5w6eT

This does not mean anything, you're not testing Node.js, and you're not even testing the same code.

I am extremely sorry

@H4ad
Copy link
Member

H4ad commented Dec 26, 2023

@sanjaiyan-dev Thanks for the PR.

Some recommendations to improve your PR:

  • you should update your first commit message based on our guidelines, my recommendation is to change to buffer: use switch statement instead if else
  • avoid unnecessary changes, the changes related to spacing you can remove all of them and keep only the changes related to the switch.

@aduh95 We already have a benchmark for buffers read (I will trigger once the lint is fixed), I don't think that's necessary to have a micro-benchmark for if/else statements.

We also have other places with perf sensitive in the code that use a switch instead of if statements.

Something that maybe could affect/help is the position of each comparison, what is more common to read, 48bits or 32bits?

@sanjaiyan-dev
Copy link
Contributor Author

@sanjaiyan-dev Thanks for the PR.

Some recommendations to improve your PR:

  • you should update your first commit message based on our guidelines, my recommendation is to change to buffer: use switch statement instead if else
  • avoid unnecessary changes, the changes related to spacing you can remove all of them and keep only the changes related to the switch.

I will make the requested changes as soon as possible. Apologies for any inconvenience caused.

@sanjaiyan-dev
Copy link
Contributor Author

#51304

@sanjaiyan-dev sanjaiyan-dev deleted the sanjaiyan-branchtable-optimize branch December 28, 2023 19:49
@sanjaiyan-dev
Copy link
Contributor Author

Hi @H4ad, I'm extremely sorry. I've created a corrected PR at #51304. I apologize for any inconvenience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants