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

http: refactor outgoing headers processing #20250

Conversation

apapirovski
Copy link
Member

Use a shared function, for..in instead of Object.keys, do less work in
setHeader and instead defer some of it until later, and other minor
changes to improve clarity, as well as a slight boost in performance.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Use a shared function, for..in instead of Object.keys, do less work in
`setHeader` and instead defer some of it until later, and other minor
changes to improve clarity, as well as a slight boost in performance.
@apapirovski apapirovski added the http Issues or PRs related to the http subsystem. label Apr 24, 2018
@apapirovski apapirovski force-pushed the patch-optimize-http-outgoing-headers branch 2 times, most recently from 024c5a3 to d909785 Compare April 24, 2018 13:56
@apapirovski apapirovski force-pushed the patch-optimize-http-outgoing-headers branch from d909785 to e21e7ff Compare April 24, 2018 14:15
@apapirovski
Copy link
Member Author

Benchmarks:

http/headers.js n=1000 duplicates=100 benchmarker='wrk'        ***     16.28 %       ±2.83% ±3.78% ±4.94%
http/headers.js n=1000 duplicates=1 benchmarker='wrk'          ***     -3.92 %       ±1.59% ±2.11% ±2.75%
http/headers.js n=10 duplicates=100 benchmarker='wrk'          ***     14.42 %       ±5.04% ±6.71% ±8.73%
http/headers.js n=10 duplicates=1 benchmarker='wrk'                     1.70 %       ±3.82% ±5.09% ±6.62%

http/set_header.js n=1000000 value='Connection'               ***      8.50 %       ±2.42% ±3.20% ±4.11%
http/set_header.js n=1000000 value='Content-Length'           ***      5.65 %       ±2.55% ±3.36% ±4.32%
http/set_header.js n=1000000 value='Content-Type'             ***      5.57 %       ±2.51% ±3.31% ±4.25%
http/set_header.js n=1000000 value='Set-Cookie'               ***      6.51 %       ±1.98% ±2.62% ±3.36%
http/set_header.js n=1000000 value='Transfer-Encoding'        ***      7.07 %       ±1.88% ±2.48% ±3.19%
http/set_header.js n=1000000 value='Vary'                     ***      9.03 %       ±2.46% ±3.24% ±4.17%
http/set_header.js n=1000000 value='X-Powered-By'             ***      5.24 %       ±2.31% ±3.04% ±3.91%

http/set-header.js res='normal' benchmarker='wrk'                      0.49 %       ±2.12% ±2.80% ±3.60%
http/set-header.js res='setHeader' benchmarker='wrk'                   0.22 %       ±2.30% ±3.03% ±3.89%
http/set-header.js res='setHeaderWH' benchmarker='wrk'                 0.48 %       ±1.94% ±2.56% ±3.28%

@lpinca lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2018
@trivikr
Copy link
Member

trivikr commented Apr 27, 2018

CI rerun for node-test-commit-osx https://ci.nodejs.org/job/node-test-commit-osx/18148/

@addaleax
Copy link
Member

Landed in 602ffd6

@addaleax addaleax closed this Apr 27, 2018
addaleax pushed a commit that referenced this pull request Apr 27, 2018
Use a shared function, for..in instead of Object.keys, do less work in
`setHeader` and instead defer some of it until later, and other minor
changes to improve clarity, as well as a slight boost in performance.

PR-URL: #20250
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@apapirovski apapirovski deleted the patch-optimize-http-outgoing-headers branch April 28, 2018 06:18
MylesBorins pushed a commit that referenced this pull request May 4, 2018
Use a shared function, for..in instead of Object.keys, do less work in
`setHeader` and instead defer some of it until later, and other minor
changes to improve clarity, as well as a slight boost in performance.

PR-URL: #20250
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants