tls: use for loop instead of forEach#24715
Conversation
There was a problem hiding this comment.
This code runs once at the point where this module is required. It'll be a completely imperceptible performance difference. Unrolling this loop altogether would make the performance optimal but doesn't mean we should do that. We should always consider magnitude of any performance improvements.
|
This change wouldn't cause any bad effect, even may make a bit of better, so why not do so ? |
Trott
left a comment
Trott
left a comment
There was a problem hiding this comment.
This makes the code less readable/maintainable for a micro-optimization that is unlikely to have any impact.
|
🤔Wondering what do you think about the proxiedMethods, these two parts are almost same with each other: |
|
I'd say make all non-critical loops |
I just don't see any reason to churn on either of these. Both are valid JavaScript that works and changing them will bring no benefit beyond adding another layer to the |
devsnek
left a comment
devsnek
left a comment
There was a problem hiding this comment.
in recent versions of v8, forEach is faster than a loop in cases like this. plus the loop is more readable.
I think for pratical reasons, all for variants perform the same (https://jsperf.com/for-vs-foreach/293). I came to prefer |
|
Thanks all of you to explain for this, let me understand a lot. If no one would like to talk more about this, please feel free to close it. |
|
I'll close this out, but thanks for the PR. |
7 participants
Using native
for loopinstead offorEachmay make the performance a bit of better ?On the other hand, it's for consistency with proxiedMethods
Refs: #11582
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes