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: switch on string values #18351

Closed
wants to merge 1 commit into from

Conversation

@sethbrenith
Copy link
Contributor

commented Jan 24, 2018

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
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.
@gireeshpunathil

This comment has been minimized.

@maclover7

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

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

@maclover7 maclover7 self-assigned this Jan 26, 2018

@maclover7

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Landed in aba6bc3

@maclover7 maclover7 closed this Jan 26, 2018

maclover7 added a commit that referenced this pull request Jan 26, 2018

http: switch on string values
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>

maclover7 added a commit that referenced this pull request Jan 26, 2018

http: switch on string values
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>

kcaulfield94 added a commit to kcaulfield94/node that referenced this pull request Feb 2, 2018

http: switch on string values
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>

@addaleax addaleax removed the author ready label Feb 4, 2018

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

http: switch on string values
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>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

http: switch on string values
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>

MylesBorins added a commit that referenced this pull request Feb 20, 2018

http: switch on string values
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 added a commit that referenced this pull request Feb 21, 2018

http: switch on string values
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 added a commit that referenced this pull request Feb 21, 2018

http: switch on string values
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 MylesBorins referenced this pull request Feb 21, 2018

Merged

v9.6.0 proposal #18902

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

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

@sethbrenith

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2018

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

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

http: switch on string values
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.