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

benchmark: net/tcp-raw-pipe.js gives wrong data #11972

Closed
vsemozhetbyt opened this issue Mar 21, 2017 · 14 comments · Fixed by #12258
Closed

benchmark: net/tcp-raw-pipe.js gives wrong data #11972

vsemozhetbyt opened this issue Mar 21, 2017 · 14 comments · Fixed by #12258
Labels
benchmark Issues and PRs related to the benchmark subsystem. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. net Issues and PRs related to the net subsystem. question Issues that look for answers. windows Issues and PRs related to the Windows platform.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Mar 21, 2017

  • Version: Node.js 8 (node-vee-eight-lkgr) or 7.7.3
  • Platform: Windows 7 x64
  • Subsystem: benchmark

After net suite completed, I've run the compare.R and get the error (translated from Russian):

Error in if (p.value < 0.001) { :
  missing value, need TRUE/FALSE
Calls: ddply ... llply -> loop_apply -> .Call -> <Anonymous> -> .fun
Aborted

The data from net/tcp-raw-pipe.js was like this:

"binary", "filename", "configuration", "rate", "time"
"old", "net\tcp-raw-pipe.js", "dur=5 type=""utf"" len=102400", 0, 5.001326985
"old", "net\tcp-raw-pipe.js", "dur=5 type=""asc"" len=102400", 0, 5.00111327
"old", "net\tcp-raw-pipe.js", "dur=5 type=""buf"" len=102400", 0, 5.001226692
"old", "net\tcp-raw-pipe.js", "dur=5 type=""utf"" len=16777216", 0, 5.012984891
"old", "net\tcp-raw-pipe.js", "dur=5 type=""asc"" len=16777216", 0, 5.000830553
"old", "net\tcp-raw-pipe.js", "dur=5 type=""buf"" len=16777216", 0, 5.001104889
"new", "net\tcp-raw-pipe.js", "dur=5 type=""utf"" len=102400", 0, 5.000816584
"new", "net\tcp-raw-pipe.js", "dur=5 type=""asc"" len=102400", 0, 5.00122753
"new", "net\tcp-raw-pipe.js", "dur=5 type=""buf"" len=102400", 0, 5.000590299
"new", "net\tcp-raw-pipe.js", "dur=5 type=""utf"" len=16777216", 0, 5.0089327
"new", "net\tcp-raw-pipe.js", "dur=5 type=""asc"" len=16777216", 0, 5.000960457
"new", "net\tcp-raw-pipe.js", "dur=5 type=""buf"" len=16777216", 0, 5.008797487
...

If I launch this benchmark separately, the output is:

net\tcp-raw-pipe.js dur=5 type="utf" len=102400: 0
net\tcp-raw-pipe.js dur=5 type="asc" len=102400: 0
net\tcp-raw-pipe.js dur=5 type="buf" len=102400: 0
net\tcp-raw-pipe.js dur=5 type="utf" len=16777216: 0
net\tcp-raw-pipe.js dur=5 type="asc" len=16777216: 0
net\tcp-raw-pipe.js dur=5 type="buf" len=16777216: 0

Is this in my environment only? Can anybody reproduce?

Update: now this benchmark throws on Windows due to this check (see #12030).

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Mar 21, 2017

cc @nodejs/benchmarking, @isaacs (the author)

@vsemozhetbyt vsemozhetbyt added benchmark Issues and PRs related to the benchmark subsystem. question Issues that look for answers. labels Mar 21, 2017
@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Mar 21, 2017
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Mar 22, 2017

It seems the cause is this:

  1. This clientHandle.onread handler is never fired (checked via simple console.log() injection).
  2. So bytes initialized by 0 here is never increased here.
  3. So bench.end() here is always called with 0 argument, causing rate to be always 0 in common.js here (update: now it throws due to this check; see benchmark: check end() argument to be > 0 #12030).

Sorry, this benchmark is filled with internal undocumented API via bindings, so I can't understand it completely to proceed on.

also cc @bnoordhuis, @indutny, @nodejs/streams (as to 'Who to CC in issues' with lib/net)

@vsemozhetbyt vsemozhetbyt added confirmed-bug Issues with confirmed bugs. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Mar 25, 2017
@joyeecheung
Copy link
Member

This looks like a windows-specific bug. I can get normal rates with this benchmark on OSX 10.11 and Ubuntu 16.04.

@joyeecheung joyeecheung added the windows Issues and PRs related to the Windows platform. label Mar 25, 2017
@vsemozhetbyt vsemozhetbyt removed the confirmed-bug Issues with confirmed bugs. label Mar 25, 2017
@vsemozhetbyt
Copy link
Contributor Author

cc @nodejs/platform-windows

@seishun
Copy link
Contributor

seishun commented Mar 25, 2017

IMO this benchmark should be removed or rewritten. Using internal APIs from process.binding doesn't exactly give reliable stats.

@bnoordhuis
Copy link
Member

It's for comparative purposes, to measure the overhead of the official API.

I don't have a Windows machine at hand right now but I can't reproduce on a Linux and OS X machine.

@seishun
Copy link
Contributor

seishun commented Mar 25, 2017

I get zeroes on Windows too.

@vsemozhetbyt BTW I suggest linking to a specific commit, since master is a moving target and the links might end up linking to the wrong lines.

@vsemozhetbyt
Copy link
Contributor Author

@seishun Thank you! I've tried to replace. I can take any last commit from the file history, right?

vsemozhetbyt added a commit that referenced this issue Mar 28, 2017
PR-URL: #12030
Ref: #11972
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 28, 2017
PR-URL: #12030
Ref: #11972
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@vsemozhetbyt
Copy link
Contributor Author

Sorry, another attempt. cc @nodejs/platform-windows, @nodejs/streams, @nodejs/performance — can anybody try to fix this for using on Windows?

@mcollina
Copy link
Member

mcollina commented Apr 5, 2017

It can take me some time to set an env to check this, as I will need to configure my windows machine for core development. So, if someone else can help here, I would be grateful.
Otherwise, it might take me some time.

@refack
Copy link
Contributor

refack commented Apr 5, 2017

@mcollina , I'll can run checks (my machine is churning core node 90% of the time ;)

@refack
Copy link
Contributor

refack commented Apr 5, 2017

@vsemozhetbyt repro

D:\code\node\benchmark>set NODEJS_BENCHMARK_ZERO_ALLOWED=1

D:\code\node\benchmark>node --version
v7.8.0

D:\code\node\benchmark>node net/tcp-raw-pipe.js benchmarker=autocannon
net\tcp-raw-pipe.js dur=5 type="utf" len=102400: 0
net\tcp-raw-pipe.js dur=5 type="asc" len=102400: 0
net\tcp-raw-pipe.js dur=5 type="buf" len=102400: 0
net\tcp-raw-pipe.js dur=5 type="utf" len=16777216: 0
net\tcp-raw-pipe.js dur=5 type="asc" len=16777216: 0
net\tcp-raw-pipe.js dur=5 type="buf" len=16777216: 0

@refack
Copy link
Contributor

refack commented Apr 6, 2017

after #12258

D:\code\node$  node benchmark\run.js --filter tcp-raw-pipe --format csv net
"filename", "configuration", "rate", "time"
"net\tcp-raw-pipe.js", "dur=5 type=""utf"" len=102400", 1.4360780042572094, 5.006651021
"net\tcp-raw-pipe.js", "dur=5 type=""asc"" len=102400", 1.0728260810032646, 5.23690301
"net\tcp-raw-pipe.js", "dur=5 type=""buf"" len=102400", 1.6941278707272742, 5.009620952
"net\tcp-raw-pipe.js", "dur=5 type=""utf"" len=16777216", 0.8651128847584556, 5.201633312
"net\tcp-raw-pipe.js", "dur=5 type=""asc"" len=16777216", 0.9211717020290138, 5.156476246
"net\tcp-raw-pipe.js", "dur=5 type=""buf"" len=16777216", 0.9165514333985011, 5.182469665

@vsemozhetbyt
Copy link
Contributor Author

@refack Thank you!

refack added a commit to refack/node that referenced this issue Apr 14, 2017
fixes nodejs#11972

PR-URL: nodejs#12258
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this issue Apr 25, 2017
fixes #11972

PR-URL: #12258
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this issue May 1, 2017
fixes #11972

PR-URL: #12258
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this issue May 2, 2017
fixes #11972

PR-URL: #12258
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Jun 18, 2017
fixes #11972

PR-URL: #12258
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Jun 20, 2017
fixes #11972

PR-URL: #12258
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 11, 2017
fixes #11972

PR-URL: #12258
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. net Issues and PRs related to the net subsystem. question Issues that look for answers. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants