FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http: improve for-loop readability in _http_outgoing.js by gengjiawen · Pull Request #26408 · nodejs/node · GitHub

/ node Public

http: improve for-loop readability in _http_outgoing.js - #26408

Closed
gengjiawen wants to merge 1 commit into
nodejs:masterfrom
gengjiawen:forloop_http_outgoing
ConversationCommits (1)ChecksFiles changed
Closed

http: improve for-loop readability in _http_outgoing.js#26408
gengjiawen wants to merge 1 commit into
nodejs:masterfrom
gengjiawen:forloop_http_outgoing

Conversation

gengjiawen commented Mar 3, 2019

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

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Mar 3, 2019
gengjiawen force-pushed the forloop_http_outgoing branch from efd378c to 4b05938 Compare March 3, 2019 05:01
gengjiawen commented Mar 3, 2019
View reviewed changes
Comment thread lib/_http_outgoing.js Outdated
gengjiawen force-pushed the forloop_http_outgoing branch from 4b05938 to bd731a0 Compare March 3, 2019 07:46
BridgeAR reviewed Mar 3, 2019
View reviewed changes
Comment thread lib/_http_outgoing.js Outdated
Comment thread lib/_http_outgoing.js Outdated
BridgeAR changed the title lib: improve for-loop readability in _http_outgoing.js http: improve for-loop readability in _http_outgoing.js Mar 5, 2019

BridgeAR commented Mar 5, 2019

Copy link
Copy Markdown
Member

Object.values() has not been improvement significantly since the last time I checked: https://bugs.chromium.org/p/v8/issues/detail?id=8071

If we change something, I suggest to use Object.keys() instead of in.

gengjiawen force-pushed the forloop_http_outgoing branch from bd731a0 to 781024d Compare March 6, 2019 14:12

gengjiawen commented Mar 6, 2019

Copy link
Copy Markdown
Member Author

@BridgeAR Can you review this again ? thanks.

BridgeAR approved these changes Mar 6, 2019
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would still check for if (headers != null) {. That way if there are no headers, we spare two extra checks.

gengjiawen commented Mar 6, 2019

Copy link
Copy Markdown
Member Author

@BridgeAR Good point. I will add it.

gengjiawen force-pushed the forloop_http_outgoing branch from 781024d to 9c0359e Compare March 6, 2019 15:21

gengjiawen commented Mar 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@BridgeAR Done.

BridgeAR commented Mar 6, 2019

Copy link
Copy Markdown
Member

CI https://ci.nodejs.org/job/node-test-pull-request/21268/

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 6, 2019
BridgeAR requested review from lpinca and mcollina March 6, 2019 23:34
mcollina requested changes Mar 7, 2019
View reviewed changes

mcollina left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is a very sensitive hot path. I’m -1 to any changes that do not improve our benchmarks.

lpinca commented Mar 7, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

I think this has 0 impact on performance. Will run a benchmark CI to verify.

mcollina commented Mar 7, 2019

Copy link
Copy Markdown
Member

@lpinca I'm pretty sure it has some impact. for-of  loops are slower than for(;;).
https://github.com/nodejs/node/pull/26408/files#diff-286202fdbdd74ede6f5f5334b6176b5cR306.

lpinca commented Mar 7, 2019

Copy link
Copy Markdown
Member

https://jsperf.com/for-of-vs-for-loop they seem to be on par on Chrome 72.

lpinca commented Mar 7, 2019

Copy link
Copy Markdown
Member

If this is a problem for 10.x 8.x and 6.x we can add the required "do not land" labels.

mscdex commented Mar 7, 2019

Copy link
Copy Markdown
Contributor

https://jsperf.com/for-of-vs-for-loop they seem to be on par on Chrome 72.

The for-loop case in that benchmark consistently shows it being faster for me with Chrome 72

gengjiawen commented Mar 7, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

On chrome 72, for-of is faster than for-loop.

Update:
sometimes, the perf is very near

Rednickle mentioned this pull request Mar 8, 2021
Open
This was referenced Mar 11, 2021
Open
Open
This was referenced May 25, 2021
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Apr 7, 2022
Open
Open
Open
Open
Open
Open
Open
Open
Closed
snyk-bot mentioned this pull request Jan 30, 2023
Open
wedataintelligence mentioned this pull request Jan 30, 2023
Open
snyk-bot mentioned this pull request Jan 30, 2023
Open
molllyn1 mentioned this pull request Jan 31, 2023
Open
snyk-bot mentioned this pull request Jan 31, 2023
Open
baby636 mentioned this pull request Jan 31, 2023
Open
This was referenced Jan 31, 2023
Open
Open
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
lpinca lpinca approved these changes
BridgeAR BridgeAR approved these changes
+1 more reviewer
mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. http Issues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL