lib: Enhancing Performance and Readability with Switch Statements 🚀 #51304
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
buffer
|
Thanks for the PR, now both changes and the commit message are good! To evaluate the perf improvement/regression of this change, I will run the following benchmark, I will post the results once it finishes (you probably will not have access to it right now). Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1488/ This can take multiple minutes/hours to finish, I will decide if I approve if there is no regression. @nodejs/performance Talking about micro-optimizations, someone has any thoughts on changing the order of |
RafaelGSS
approved these changes
Dec 28, 2023
RafaelGSS
added
the
performance
Welcome! By the way, I believe the switch statement uses a branch table, so unlike an if statement, the order doesn't matter. (I'm sorry if I'm mistaken.) |
anonrig
requested changes
Dec 28, 2023
| case '[object Float32Array]': return 7; | ||
| case '[object Float64Array]': return 8; | ||
| case '[object DataView]': return 9; | ||
| // Index 10 id FastBuffer |
There was a problem hiding this comment.
Suggested change
| // Index 10 id FastBuffer | |
| // Index 10 is FastBuffer |
|
I am confused. I would expect that putting some stuff in an array is faster. |
mcollina
approved these changes
Dec 29, 2023
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1488/ It doesn't look like a clear win |
|
Looks more like an overall loss. |
|
Even if the results are not positive, at least they are interesting, there is something on v8 that is making if statements faster than switch statements? If this is right, maybe we should take a look at another place where we use @mcollina @RafaelGSS do you guys have any take on these results? Otherwise, I think we should close this one. @sanjaiyan-dev Do the benchmarks make sense to you? In case you want to run those benchmarks locally to try understand why it was slower, you can follow the guide of benchmarks and then run buffer-read-with-byteLength.js, which is the benchmark that is more impacted by your change. |
FWIW, such micro-optimizations are things that we'd have to benchmark after every V8 update, possibly even on every supported architecture. |
|
I actually dont understand why switch cases where we check for integers are not put in an array. |
Hello, certainly, I will attempt to run the benchmarks on my local machine. I apologize for any inconvenience caused. |
I sincerely apologize; I couldn't understand you. |
|
@H4ad hint - run the code through systems analyzer and see the IR difference: https://v8.github.io/tools/head/system-analyzer/index.html I don't find this version more readable and as it is also measurably slower I don't think we should merge this. Thanks for the PR though <3 |
I would definitely look at benchmarking the array index lookup (which might not work for certain functions but I feel could increase the speed for functions where possible). Here's an example test (I know not necessarily real-world but the results were interesting). Output was the following on local hardware: (ps: I took out Fastbuffer locally as I didn't have it, this was really just a raw test because I was curious). Hope it helps :)! script: const idxmap = new Map([
[0, Int8Array],
[1, Uint8Array],
[2, Uint8ClampedArray],
[3, Int16Array],
[4, Uint16Array],
[5, Int32Array],
[6, Uint32Array],
[7, Float32Array],
[8, Float64Array],
[9, DataView],
[11, BigInt64Array],
[12, BigUint64Array],
]);
const idxarr = [
Int8Array,
Uint8Array,
Uint8ClampedArray,
Int16Array,
Uint16Array,
Int32Array,
Uint32Array,
Float32Array,
Float64Array,
DataView,
BigInt64Array,
BigUint64Array,
];
function usingIf (idx) {
if (idx === 0) return Int8Array;
if (idx === 1) return Uint8Array;
if (idx === 2) return Uint8ClampedArray;
if (idx === 3) return Int16Array;
if (idx === 4) return Uint16Array;
if (idx === 5) return Int32Array;
if (idx === 6) return Uint32Array;
if (idx === 7) return Float32Array;
if (idx === 8) return Float64Array;
if (idx === 9) return DataView;
if (idx === 11) return BigInt64Array;
if (idx === 12) return BigUint64Array;
return undefined;
}
function usingSwitch (idx) {
switch (idx) {
case 0: return Int8Array;
case 1: return Uint8Array;
case 2: return Uint8ClampedArray;
case 3: return Int16Array;
case 4: return Uint16Array;
case 5: return Int32Array;
case 6: return Uint32Array;
case 7: return Float32Array;
case 8: return Float64Array;
case 9: return DataView;
case 11: return BigInt64Array;
case 12: return BigUint64Array;
default: return undefined;
}
}
function usingMap (idx) {
return idxmap.get(idx) || undefined;
}
function usingArray (idx) {
return idxarr[idx] || undefined;
}
// Bench util
const ROW_TEST_WIDTH = 60;
const ROW_OPS_WIDTH = 20;
const ITERATIONS = 100000000;
function separator () {
console.info(''.padEnd(ROW_TEST_WIDTH + ROW_OPS_WIDTH, '-'));
}
function bench (el) {
let runtime = performance.now();
for (let i = 0; i < ITERATIONS; i++) el.fn(9);
runtime = performance.now() - runtime;
const ops = Math.floor(ITERATIONS * (1000/runtime));
console.info([el.lbl.padEnd(ROW_TEST_WIDTH, ' '), `${ops}`.padEnd(ROW_OPS_WIDTH, ' ')].join('| '));
}
bench({fn: usingArray, lbl: 'Using Array'});
bench({fn: usingIf, lbl: 'Using If'});
bench({fn: usingMap, lbl: 'Using Map'});
bench({fn: usingSwitch, lbl: 'Using Switch'}); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is the fixed version (pull request) of #51281