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

http: switch on string values by sethbrenith · Pull Request #18351 · nodejs/node · GitHub

/ node Public

http: switch on string values - #18351

Closed
sethbrenith wants to merge 1 commit into
nodejs:masterfrom
sethbrenith:string-switch
ConversationCommits (1)ChecksFiles changed
Closed

http: switch on string values#18351
sethbrenith wants to merge 1 commit into
nodejs:masterfrom
sethbrenith:string-switch

Conversation

sethbrenith commented Jan 24, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Long ago, V8 was much faster switching on string lengths than values. That is no longer the case, so we can simplify a couple of methods.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes (sort of, see detail below)
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • http
Test results

I ran the test suite and found the following errors on Windows, which were consistent with or without this change. A couple of other errors also showed up inconsistently. Two of them seem to be covered by 18251, but I didn't see any active issue for test-assert.

=== release test-assert ===
Path: parallel/test-assert
assert.js:68
  throw new errors.AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: 'The expression evaluated to a falsy value:\r\n\r\n  assert((() => \'string\')()\n      // eslint-disable-next-line\n      ===\n strictEqual 'The expression evaluated to a falsy value:\r\n\r\n  assert((() => \'string\')()\r\n      // eslint-disable-next-line\r\n      =
    at Object.innerFn (D:\repos\node\test\common\index.js:762:16)
    at expectedException (assert.js:374:19)
    at Function.throws (assert.js:419:16)
    at Object.expectsError (D:\repos\node\test\common\index.js:784:12)
    at Object.<anonymous> (D:\repos\node\test\parallel\test-assert.js:842:8)
    at Module._compile (module.js:661:30)
    at Object.Module._extensions..js (module.js:672:10)
    at Module.load (module.js:578:32)
    at tryModuleLoad (module.js:518:12)
    at Function.Module._load (module.js:510:3)
Command: D:\repos\node\Release\node.exe D:\repos\node\test\parallel\test-assert.js
=== release test-http2-ping-flood ===
Path: sequential/test-http2-ping-flood
(node:23136) ExperimentalWarning: The http2 module is an experimental API.
Command: D:\repos\node\Release\node.exe D:\repos\node\test\sequential\test-http2-ping-flood.js
--- TIMEOUT ---
=== release test-http2-settings-flood ===
Path: sequential/test-http2-settings-flood
(node:30612) ExperimentalWarning: The http2 module is an experimental API.
Command: D:\repos\node\Release\node.exe D:\repos\node\test\sequential\test-http2-settings-flood.js
--- TIMEOUT ---
Benchmark results
>Release\node.exe benchmark\compare.js --old Release_old\node.exe --new Release\node.exe --filter set_header http | rscript benchmark\compare.R
[00:05:25|% 100| 1/1 files | 60/60 runs | 7/7 configs]: Done
                                                          improvement confidence      p.value
 http\\set_header.js n=1000000 value="Connection"            22.61 %        *** 5.803813e-14
 http\\set_header.js n=1000000 value="Content-Length"        20.64 %        *** 9.065010e-19
 http\\set_header.js n=1000000 value="Content-Type"          23.51 %        *** 6.291114e-26
 http\\set_header.js n=1000000 value="Set-Cookie"            26.20 %        *** 8.024882e-29
 http\\set_header.js n=1000000 value="Transfer-Encoding"     15.54 %        *** 6.562425e-14
 http\\set_header.js n=1000000 value="Vary"                  20.31 %        *** 1.390219e-17
 http\\set_header.js n=1000000 value="X-Powered-By"          21.68 %        *** 7.662119e-17

http: switch on string values
21fa397 
Long ago, V8 was much faster switching on string lengths than values.
That is no longer the case, so we can simplify a couple of methods.
nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jan 24, 2018

gireeshpunathil commented Jan 25, 2018

Copy link
Copy Markdown
Member

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

maclover7 commented Jan 26, 2018

Copy link
Copy Markdown
Contributor

All CI failures on Windows are either fixed, or known flakey tests. Landing.

maclover7 self-assigned this Jan 26, 2018

maclover7 commented Jan 26, 2018

Copy link
Copy Markdown
Contributor

Landed in aba6bc3

maclover7 closed this Jan 26, 2018
maclover7 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 26, 2018
maclover7 pushed a commit that referenced this pull request Jan 26, 2018
http: switch on string values
aba6bc3 
Long ago, V8 was much faster switching on string lengths than values.
That is no longer the case, so we can simplify a couple of methods.

PR-URL: #18351
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 4, 2018
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
http: switch on string values
384d4e4 
Long ago, V8 was much faster switching on string lengths than values.
That is no longer the case, so we can simplify a couple of methods.

PR-URL: #18351
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
http: switch on string values
bd2f210 
Long ago, V8 was much faster switching on string lengths than values.
That is no longer the case, so we can simplify a couple of methods.

PR-URL: #18351
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
http: switch on string values
d764039 
Long ago, V8 was much faster switching on string lengths than values.
That is no longer the case, so we can simplify a couple of methods.

PR-URL: #18351
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins mentioned this pull request Feb 21, 2018
Merged

MylesBorins commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

Should this land on v6.x or v8.x? It lands cleanly on 8.x but will require a manual backport to 6

sethbrenith commented Mar 20, 2018

Copy link
Copy Markdown
Contributor Author

I think probably not, because V8 performance characteristics have changed over time and we wouldn't want to regress old versions.

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
http: switch on string values
ee6340b 
Long ago, V8 was much faster switching on string lengths than values.
That is no longer the case, so we can simplify a couple of methods.

PR-URL: nodejs#18351
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
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

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
gireeshpunathil gireeshpunathil approved these changes
+3 more reviewers
apapirovski apapirovski approved these changes
kfarnung kfarnung approved these changes
maclover7 maclover7 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

maclover7

Labels

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.

10 participants

Back | FazBrowse Home | New Git URL