http2: set decodeStrings to false, test for createWriteReq #15140

apapirovski · 2017-09-02T00:22:36Z

Adds a test for all the encodings in createWriteReq, including the default case. The other option was mocking all the handlers but that didn't seem any better. Also, don't think there's a way to set objectMode without digging into _writableState.

If anyone has better ideas on how to properly test this, I'm happy to adjust. Thanks!

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

http2, test

benjamingr

Not sure if this is how I would test it - but overall LGTM. Thank you for making a contribution!

apapirovski · 2017-09-02T12:21:45Z

@benjamingr If you have suggestions, I would be happy to implement/take into consideration for the future. Thanks!

mcollina · 2017-09-05T17:22:31Z

I am a bit lost on why the objectMode = true need to be tested. Is it needed to validate the encodings bit? In that case it should be left out from the commit message, the title of the PR and put a comment on top on the test itself.

apapirovski · 2017-09-05T18:38:36Z

@mcollina it converts to Buffer before it gets to createWriteReq without objectMode = true. It only hits those code paths if it's in that mode so it felt like it was key to mention but I can adjust the commit message.

mcollina · 2017-09-06T09:15:28Z

Can you point me to this code path?

apapirovski · 2017-09-06T11:55:56Z

Here's the full chain that leads to this:

node/lib/_stream_writable.js

Lines 264 to 267 in 1403d28

    
           Writable.prototype.write = function(chunk, encoding, cb) { 
        
             var state = this._writableState; 
        
             var ret = false; 
        
             var isBuf = !state.objectMode && Stream._isUint8Array(chunk);

node/lib/_stream_writable.js

Lines 339 to 347 in 1403d28

    
           function writeOrBuffer(stream, state, isBuf, chunk, encoding, cb) { 
        
             if (!isBuf) { 
        
               var newChunk = decodeChunk(state, chunk, encoding); 
        
               if (chunk !== newChunk) { 
        
                 isBuf = true; 
        
                 encoding = 'buffer'; 
        
                 chunk = newChunk; 
        
               } 
        
             }

node/lib/_stream_writable.js

Line 387 in 1403d28

stream._write(chunk, encoding, state.onwrite);

node/lib/_stream_writable.js

Lines 311 to 316 in d111319

    
           function decodeChunk(state, chunk, encoding) { 
        
             if (!state.objectMode && 
        
                 state.decodeStrings !== false && 
        
                 typeof chunk === 'string') { 
        
               chunk = Buffer.from(chunk, encoding); 
        
             }

I could've also used decodeStrings: false but either way I have to set it through _writableState. Although it probably makes it clearer what's going on.

apapirovski · 2017-09-06T12:18:17Z

Updated the commit message and the comment in the test itself.

mcollina · 2017-09-06T12:19:07Z

It seems al of these are releated to streams, is there an http2 part of this? Otherwise, let's just write a test for streams.

apapirovski · 2017-09-06T12:33:02Z

Sorry, should've been clearer. That's just the code leading up to our _write.

node/lib/internal/http2/core.js

Line 1384 in 1403d28

const err = createWriteReq(req, handle, data, encoding);

node/lib/internal/http2/core.js

Lines 1157 to 1177 in 1403d28

    
           function createWriteReq(req, handle, data, encoding) { 
        
             switch (encoding) { 
        
               case 'latin1': 
        
               case 'binary': 
        
                 return handle.writeLatin1String(req, data); 
        
               case 'buffer': 
        
                 return handle.writeBuffer(req, data); 
        
               case 'utf8': 
        
               case 'utf-8': 
        
                 return handle.writeUtf8String(req, data); 
        
               case 'ascii': 
        
                 return handle.writeAsciiString(req, data); 
        
               case 'ucs2': 
        
               case 'ucs-2': 
        
               case 'utf16le': 
        
               case 'utf-16le': 
        
                 return handle.writeUcs2String(req, data); 
        
               default: 
        
                 return handle.writeBuffer(req, Buffer.from(data, encoding)); 
        
             } 
        
           }

Usually we only hit the 'buffer' case, hence switching to objectMode or turning off decodeStrings.

mcollina · 2017-09-06T13:16:43Z

So, we should be able to receive strings straight into HTTP2. By moving them to buffers we are losing throughput.
I think we should turn off decodeStrings in HTTP2 core (not for the tests). We have a fast path to writing those straight to the HTTP2Stream, let's use it.

apapirovski · 2017-09-06T13:18:35Z

@mcollina Yep, I already started working on PR after this conversation. Thanks for the review!

Would it make sense to just update this PR for it or would you prefer a new one?

apapirovski · 2017-09-06T16:49:34Z

Ok, everything has been updated. I added a benchmark too. First benchmark so let me know if I should change anything. It's a substantial speed up btw.

Old:

http2/write.js length=65536 streams=100 requests=100 benchmarker="h2load": 1,040.47
http2/write.js length=131072 streams=100 requests=100 benchmarker="h2load": 0.00
http2/write.js length=262144 streams=100 requests=100 benchmarker="h2load": 342.20
http2/write.js length=1048576 streams=100 requests=100 benchmarker="h2load": 89.34
http2/write.js length=65536 streams=200 requests=100 benchmarker="h2load": 1,117.59
http2/write.js length=131072 streams=200 requests=100 benchmarker="h2load": 623.94
http2/write.js length=262144 streams=200 requests=100 benchmarker="h2load": 331.93
http2/write.js length=1048576 streams=200 requests=100 benchmarker="h2load": 89.68
http2/write.js length=65536 streams=1000 requests=100 benchmarker="h2load": 1,105.97
http2/write.js length=131072 streams=1000 requests=100 benchmarker="h2load": 656.71
http2/write.js length=262144 streams=1000 requests=100 benchmarker="h2load": 351.67
http2/write.js length=1048576 streams=1000 requests=100 benchmarker="h2load": 85.21
http2/write.js length=65536 streams=100 requests=1000 benchmarker="h2load": 1,279.40
http2/write.js length=131072 streams=100 requests=1000 benchmarker="h2load": 80.07
http2/write.js length=262144 streams=100 requests=1000 benchmarker="h2load": 381.78
http2/write.js length=1048576 streams=100 requests=1000 benchmarker="h2load": 46.25
http2/write.js length=65536 streams=200 requests=1000 benchmarker="h2load": 1,302.14
http2/write.js length=131072 streams=200 requests=1000 benchmarker="h2load": 746.88
http2/write.js length=262144 streams=200 requests=1000 benchmarker="h2load": 386.10
http2/write.js length=1048576 streams=200 requests=1000 benchmarker="h2load": 0.00
http2/write.js length=65536 streams=1000 requests=1000 benchmarker="h2load": 1,096.86
http2/write.js length=131072 streams=1000 requests=1000 benchmarker="h2load": 704.04
http2/write.js length=262144 streams=1000 requests=1000 benchmarker="h2load": 368.96
http2/write.js length=1048576 streams=1000 requests=1000 benchmarker="h2load": 80.00
http2/write.js length=65536 streams=100 requests=10000 benchmarker="h2load": 591.57
http2/write.js length=131072 streams=100 requests=10000 benchmarker="h2load": 580.47
http2/write.js length=262144 streams=100 requests=10000 benchmarker="h2load": 351.71
http2/write.js length=1048576 streams=100 requests=10000 benchmarker="h2load": 78.56
http2/write.js length=65536 streams=200 requests=10000 benchmarker="h2load": 1,118.39
http2/write.js length=131072 streams=200 requests=10000 benchmarker="h2load": 693.63
http2/write.js length=262144 streams=200 requests=10000 benchmarker="h2load": 376.24
http2/write.js length=1048576 streams=200 requests=10000 benchmarker="h2load": 70.16
http2/write.js length=65536 streams=1000 requests=10000 benchmarker="h2load": 1,114.98
http2/write.js length=131072 streams=1000 requests=10000 benchmarker="h2load": 612.16
http2/write.js length=262144 streams=1000 requests=10000 benchmarker="h2load": 280.43
http2/write.js length=1048576 streams=1000 requests=10000 benchmarker="h2load": 68.78

New:

http2/write.js length=65536 streams=100 requests=100 benchmarker="h2load": 1,431.64
http2/write.js length=131072 streams=100 requests=100 benchmarker="h2load": 848.23
http2/write.js length=262144 streams=100 requests=100 benchmarker="h2load": 460.35
http2/write.js length=1048576 streams=100 requests=100 benchmarker="h2load": 125.00
http2/write.js length=65536 streams=200 requests=100 benchmarker="h2load": 1,407.76
http2/write.js length=131072 streams=200 requests=100 benchmarker="h2load": 821.02
http2/write.js length=262144 streams=200 requests=100 benchmarker="h2load": 488.43
http2/write.js length=1048576 streams=200 requests=100 benchmarker="h2load": 121.48
http2/write.js length=65536 streams=1000 requests=100 benchmarker="h2load": 1,464.69
http2/write.js length=131072 streams=1000 requests=100 benchmarker="h2load": 848.33
http2/write.js length=262144 streams=1000 requests=100 benchmarker="h2load": 469.26
http2/write.js length=1048576 streams=1000 requests=100 benchmarker="h2load": 124.49
http2/write.js length=65536 streams=100 requests=1000 benchmarker="h2load": 1,692.65
http2/write.js length=131072 streams=100 requests=1000 benchmarker="h2load": 954.82
http2/write.js length=262144 streams=100 requests=1000 benchmarker="h2load": 536.26
http2/write.js length=1048576 streams=100 requests=1000 benchmarker="h2load": 127.43
http2/write.js length=65536 streams=200 requests=1000 benchmarker="h2load": 1,602.40
http2/write.js length=131072 streams=200 requests=1000 benchmarker="h2load": 980.88
http2/write.js length=262144 streams=200 requests=1000 benchmarker="h2load": 583.23
http2/write.js length=1048576 streams=200 requests=1000 benchmarker="h2load": 135.87
http2/write.js length=65536 streams=1000 requests=1000 benchmarker="h2load": 1,503.10
http2/write.js length=131072 streams=1000 requests=1000 benchmarker="h2load": 877.49
http2/write.js length=262144 streams=1000 requests=1000 benchmarker="h2load": 494.46
http2/write.js length=1048576 streams=1000 requests=1000 benchmarker="h2load": 120.87
http2/write.js length=65536 streams=100 requests=10000 benchmarker="h2load": 1,579.05
http2/write.js length=131072 streams=100 requests=10000 benchmarker="h2load": 984.19
http2/write.js length=262144 streams=100 requests=10000 benchmarker="h2load": 496.87
http2/write.js length=1048576 streams=100 requests=10000 benchmarker="h2load": 109.32
http2/write.js length=65536 streams=200 requests=10000 benchmarker="h2load": 1,799.27
http2/write.js length=131072 streams=200 requests=10000 benchmarker="h2load": 990.87
http2/write.js length=262144 streams=200 requests=10000 benchmarker="h2load": 538.14
http2/write.js length=1048576 streams=200 requests=10000 benchmarker="h2load": 134.92
http2/write.js length=65536 streams=1000 requests=10000 benchmarker="h2load": 1,797.03
http2/write.js length=131072 streams=1000 requests=10000 benchmarker="h2load": 965.08
http2/write.js length=262144 streams=1000 requests=10000 benchmarker="h2load": 543.61
http2/write.js length=1048576 streams=1000 requests=10000 benchmarker="h2load": 133.38

The format of these tests doesn't seem compatible with the compare script but it looks like a 35% speed up even if I'm being conservative, maybe higher.

mcollina · 2017-09-06T17:30:38Z

@benjamingr @claudiorodriguez this would require a new review.

mcollina

LGTM

mcollina · 2017-09-06T17:31:39Z

@apapirovski you should follow this guide to compare two nodejs versions https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md#comparing-nodejs-versions

apapirovski · 2017-09-06T17:33:16Z

@mcollina ~~it doesn't seem to work for benchmarks using h2load for some reason~~ Actually, what I'm having trouble is getting it to load the different benchmarker with compare

Update: Got it to work. Forgot "--set"... *sigh* totally my bad.

mcollina · 2017-09-06T19:24:32Z

which command did you use in the end?

apapirovski · 2017-09-06T19:33:15Z

This is what I used

node benchmark/compare.js --runs 3 --old ./out/Release-old/node --new ./out/Release/node --filter write --set benchmarker=h2load http2 > results.csv

I was just forgetting --set hence my confusion above.

I got about 45% improvement across the board but didn't have time to keep running it for more than 3 runs. Might run it for longer later but I think it's safe to say this is a big improvement.

mcollina · 2017-09-07T16:42:58Z

Ci: https://ci.nodejs.org/job/node-test-pull-request/9982/

apapirovski · 2017-09-07T17:04:02Z

linux-one failed on the test case which I think might mean there's an issue in the underlying C++ code. @jasnell @mcollina can one of you take a look maybe? This is a bit outside of my area of expertise.

mcollina · 2017-09-07T17:06:47Z

I think it's a problem of having a timeout-based test:

not ok 800 parallel/test-http2-createwritereq
  ---
  duration_ms: 0.306
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected exactly 11, actual 8.
        at Object.exports.mustCall (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:477:10)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-http2-createwritereq.js:29:42)
        at Module._compile (module.js:549:30)
        at Object.Module._extensions..js (module.js:560:10)
        at Module.load (module.js:483:32)
        at tryModuleLoad (module.js:446:12)
        at Function.Module._load (module.js:438:3)
        at Function.Module.runMain (module.js:585:10)
        at startup (bootstrap_node.js:194:16)
    (node:34482) ExperimentalWarning: The http2 module is an experimental API.

Those platforms are slow :(

apapirovski · 2017-09-07T17:08:08Z

@mcollina this one isn't timeout based, this one just writes strings and expects to get the right thing on the other end. Although I think it does have to do with the platform being slow. I think it closes the server before it received everything. Will adjust the test case to be less flaky. Good lesson for me re: writing tests for slower platforms, have to make less assumptions about what is finished and what isn't.

Set writableStream decodeStrings to false to let the native layer handle converting strings to buffer.

apapirovski · 2017-09-07T17:22:00Z

@mcollina I think it should work now. Could we restart the CI? Thanks.

mcollina · 2017-09-07T17:44:37Z

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

mcollina · 2017-09-07T17:46:04Z

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

mcollina · 2017-09-07T21:33:27Z

CI is green, this is ok to go.

Set writableStream decodeStrings to false to let the native layer handle converting strings to buffer. PR-URL: #15140 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Claudio Rodriguez <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell · 2017-09-07T23:10:17Z

Landed in 2ffc8ac

Set writableStream decodeStrings to false to let the native layer handle converting strings to buffer. PR-URL: #15140 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Claudio Rodriguez <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

Set writableStream decodeStrings to false to let the native layer handle converting strings to buffer. PR-URL: nodejs#15140 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Claudio Rodriguez <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 2, 2017

apapirovski force-pushed the patch-test-http2-createwritereq branch from 1a54c36 to 0b494e6 Compare September 2, 2017 00:23

mscdex added the http2 Issues or PRs related to the http2 subsystem. label Sep 2, 2017

benjamingr approved these changes Sep 2, 2017

View reviewed changes

claudiorodriguez approved these changes Sep 3, 2017

View reviewed changes

jasnell requested a review from mcollina September 5, 2017 17:05

apapirovski changed the title ~~test: http2 test for createWriteReq in objectMode~~ test: http2 test for createWriteReq Sep 6, 2017

apapirovski force-pushed the patch-test-http2-createwritereq branch from 0b494e6 to 9a231b2 Compare September 6, 2017 12:16

apapirovski changed the title ~~test: http2 test for createWriteReq~~ test: add http2 test for createWriteReq Sep 6, 2017

apapirovski force-pushed the patch-test-http2-createwritereq branch from 9a231b2 to 7598fb4 Compare September 6, 2017 16:42

apapirovski changed the title ~~test: add http2 test for createWriteReq~~ http2: set decodeStrings to false, test for createWriteReq Sep 6, 2017

mcollina approved these changes Sep 6, 2017

View reviewed changes

jasnell approved these changes Sep 6, 2017

View reviewed changes

http2: set decodeStrings to false, test

7bd0606

Set writableStream decodeStrings to false to let the native layer handle converting strings to buffer.

apapirovski force-pushed the patch-test-http2-createwritereq branch from 7598fb4 to 7bd0606 Compare September 7, 2017 17:20

jasnell closed this Sep 7, 2017

apapirovski deleted the patch-test-http2-createwritereq branch September 9, 2017 19:57

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: set decodeStrings to false, test for createWriteReq #15140

http2: set decodeStrings to false, test for createWriteReq #15140

apapirovski commented Sep 2, 2017 •

edited

Loading

benjamingr left a comment •

edited

Loading

apapirovski commented Sep 2, 2017 •

edited

Loading

mcollina commented Sep 5, 2017

apapirovski commented Sep 5, 2017

mcollina commented Sep 6, 2017

apapirovski commented Sep 6, 2017 •

edited

Loading

apapirovski commented Sep 6, 2017

mcollina commented Sep 6, 2017

apapirovski commented Sep 6, 2017

mcollina commented Sep 6, 2017

apapirovski commented Sep 6, 2017 •

edited

Loading

apapirovski commented Sep 6, 2017

mcollina commented Sep 6, 2017

mcollina left a comment

mcollina commented Sep 6, 2017

apapirovski commented Sep 6, 2017 •

edited

Loading

mcollina commented Sep 6, 2017

apapirovski commented Sep 6, 2017

mcollina commented Sep 7, 2017

apapirovski commented Sep 7, 2017

mcollina commented Sep 7, 2017

apapirovski commented Sep 7, 2017 •

edited

Loading

apapirovski commented Sep 7, 2017

mcollina commented Sep 7, 2017

mcollina commented Sep 7, 2017

mcollina commented Sep 7, 2017

jasnell commented Sep 7, 2017

http2: set decodeStrings to false, test for createWriteReq #15140

http2: set decodeStrings to false, test for createWriteReq #15140

Conversation

apapirovski commented Sep 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

benjamingr left a comment • edited Loading

Choose a reason for hiding this comment

apapirovski commented Sep 2, 2017 • edited Loading

mcollina commented Sep 5, 2017

apapirovski commented Sep 5, 2017

mcollina commented Sep 6, 2017

apapirovski commented Sep 6, 2017 • edited Loading

apapirovski commented Sep 6, 2017

mcollina commented Sep 6, 2017

apapirovski commented Sep 6, 2017

mcollina commented Sep 6, 2017

apapirovski commented Sep 6, 2017 • edited Loading

apapirovski commented Sep 6, 2017

mcollina commented Sep 6, 2017

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Sep 6, 2017

apapirovski commented Sep 6, 2017 • edited Loading

mcollina commented Sep 6, 2017

apapirovski commented Sep 6, 2017

mcollina commented Sep 7, 2017

apapirovski commented Sep 7, 2017

mcollina commented Sep 7, 2017

apapirovski commented Sep 7, 2017 • edited Loading

apapirovski commented Sep 7, 2017

mcollina commented Sep 7, 2017

mcollina commented Sep 7, 2017

mcollina commented Sep 7, 2017

jasnell commented Sep 7, 2017

apapirovski commented Sep 2, 2017 •

edited

Loading

benjamingr left a comment •

edited

Loading

apapirovski commented Sep 2, 2017 •

edited

Loading

apapirovski commented Sep 6, 2017 •

edited

Loading

apapirovski commented Sep 6, 2017 •

edited

Loading

apapirovski commented Sep 6, 2017 •

edited

Loading

apapirovski commented Sep 7, 2017 •

edited

Loading