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.

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

tls: use for loop instead of forEach #24715

Closed
wants to merge 1 commit into from

Conversation

ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Nov 29, 2018

Using native for loop instead of forEach may make the performance a bit of better ?

On the other hand, it's for consistency with proxiedMethods

Refs: #11582

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

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Nov 29, 2018
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

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.

@ZYSzys
Copy link
Member Author

ZYSzys commented Nov 29, 2018

This change wouldn't cause any bad effect, even may make a bit of better, so why not do so ?

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.

This makes the code less readable/maintainable for a micro-optimization that is unlikely to have any impact.

@ZYSzys
Copy link
Member Author

ZYSzys commented Nov 29, 2018

🤔Wondering what do you think about the proxiedMethods, these two parts are almost same with each other:
Proxy HandleWrap, PipeWrap and TCPWrap methods
screen shot 2018-11-29 at 6 15 23 pm
Proxy TLSSocket handle methods:
screen shot 2018-11-29 at 6 16 35 pm

@silverwind
Copy link
Contributor

silverwind commented Nov 29, 2018

I'd say make all non-critical loops for...of. But such refactors should be done at per-file level at minimum, certainly not on a per-loop level.

@apapirovski
Copy link
Member

Wondering what do you think about the proxiedMethods, these two parts are almost same with each other

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 blame output that someone will have to puzzle through when trying to actually work on that code.

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

in recent versions of v8, forEach is faster than a loop in cases like this. plus the loop is more readable.

@silverwind
Copy link
Contributor

in recent versions of v8, forEach is faster

I think for pratical reasons, all for variants perform the same (https://jsperf.com/for-vs-foreach/293). I came to prefer for...of recentely because (unlike forEach) it allows the use of control statements like return, break and continue.

@ZYSzys
Copy link
Member Author

ZYSzys commented Nov 29, 2018

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.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 30, 2018

I'll close this out, but thanks for the PR.

@cjihrig cjihrig closed this Nov 30, 2018
@ZYSzys ZYSzys deleted the tls-wrap-for branch November 30, 2018 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants