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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

gengjiawen
Copy link
Member

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

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Mar 3, 2019
@gengjiawen gengjiawen force-pushed the forloop_http_outgoing branch from efd378c to 4b05938 Compare March 3, 2019 05:01
gengjiawen
gengjiawen commented Mar 3, 2019
View reviewed changes
lib/_http_outgoing.js Outdated Show resolved Hide resolved
@gengjiawen gengjiawen force-pushed the forloop_http_outgoing branch from 4b05938 to bd731a0 Compare March 3, 2019 07:46
BridgeAR
BridgeAR reviewed Mar 3, 2019
View reviewed changes
lib/_http_outgoing.js Outdated Show resolved Hide resolved
lib/_http_outgoing.js Show resolved Hide resolved
@BridgeAR 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
Copy link
Member

BridgeAR commented Mar 5, 2019

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 gengjiawen force-pushed the forloop_http_outgoing branch from bd731a0 to 781024d Compare March 6, 2019 14:12
@gengjiawen
Copy link
Member Author

@BridgeAR Can you review this again ? thanks.

BridgeAR
BridgeAR approved these changes Mar 6, 2019
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

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

@gengjiawen
Copy link
Member Author

@BridgeAR Good point. I will add it.

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

gengjiawen commented Mar 6, 2019 •
edited
Loading

@BridgeAR Done.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2019

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

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

@mcollina mcollina left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

@lpinca
Copy link
Member

lpinca commented Mar 7, 2019 •
edited
Loading

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

@mcollina
Copy link
Member

mcollina commented Mar 7, 2019

@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
Copy link
Member

lpinca commented Mar 7, 2019

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

@lpinca
Copy link
Member

lpinca commented Mar 7, 2019

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

@mscdex
Copy link
Contributor

mscdex commented Mar 7, 2019

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
Copy link
Member Author

gengjiawen commented Mar 7, 2019 •
edited
Loading

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

Update:
sometimes, the perf is very near
image

@Rednickle 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 snyk-bot mentioned this pull request Jan 30, 2023
Open
@wedataintelligence wedataintelligence mentioned this pull request Jan 30, 2023
Open
@snyk-bot snyk-bot mentioned this pull request Jan 30, 2023
Open
@molllyn1 molllyn1 mentioned this pull request Jan 31, 2023
Open
@snyk-bot snyk-bot mentioned this pull request Jan 31, 2023
Open
@baby636 baby636 mentioned this pull request Jan 31, 2023
Open
This was referenced Jan 31, 2023
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@gengjiawen @BridgeAR @lpinca @mcollina @mscdex @nodejs-github-bot