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: readStart only after completion #12258

Merged
merged 1 commit into from
Apr 14, 2017
Merged

Conversation

refack
Copy link
Contributor

@refack refack commented Apr 6, 2017

fix #11972
On windows apparently it's meaningful to call readStart only after connection completion.
(makes sense on POSIX as well, and causes no regressions)

Checklist
  • tests and/or benchmarks are included
  • commit message follows [commit guidelines]
Affected core subsystem(s)

benchmarks

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. net Issues and PRs related to the net subsystem. labels Apr 6, 2017
@refack
Copy link
Contributor Author

refack commented Apr 6, 2017

@indutny might be related to #6404
/cc @yosuke-furukawa , @bnoordhuis

@bzoz
Copy link
Contributor

bzoz commented Apr 6, 2017

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 6, 2017

@refack It is strange: test-benchmark-net.js is OK with it. But with normal run this benchmark hangs after all tests:

J:\temp\_git\node-fork\benchmark\net>set NODEJS_BENCHMARK_ZERO_ALLOWED=1

J:\temp\_git\node-fork\benchmark\net>tcp-raw-pipe.js dur=0
net\tcp-raw-pipe.js dur=0 type="utf" len=102400: 0
net\tcp-raw-pipe.js dur=0 type="asc" len=102400: 0
net\tcp-raw-pipe.js dur=0 type="buf" len=102400: 0
net\tcp-raw-pipe.js dur=0 type="utf" len=16777216: 0
net\tcp-raw-pipe.js dur=0 type="asc" len=16777216: 0
net\tcp-raw-pipe.js dur=0 type="buf" len=16777216: 0

J:\temp\_git\node-fork\benchmark\net> // OK
J:\temp\_git\node-fork\benchmark\net>set NODEJS_BENCHMARK_ZERO_ALLOWED=

J:\temp\_git\node-fork\benchmark\net>tcp-raw-pipe.js
net\tcp-raw-pipe.js dur=5 type="utf" len=102400: 1.297886419487734
net\tcp-raw-pipe.js dur=5 type="asc" len=102400: 1.0694375760443917
net\tcp-raw-pipe.js dur=5 type="buf" len=102400: 1.2243643596337568
net\tcp-raw-pipe.js dur=5 type="utf" len=16777216: 1.0074100180585681
net\tcp-raw-pipe.js dur=5 type="asc" len=16777216: 1.2771849558116457
net\tcp-raw-pipe.js dur=5 type="buf" len=16777216: 1.1929763144473262
// HUNG! Needs CTRL+C to abort

Windows 7 x64.

@refack
Copy link
Contributor Author

refack commented Apr 6, 2017

failure was because the #12261 issue/fix
IMHO can be considered green 😜

@refack
Copy link
Contributor Author

refack commented Apr 6, 2017

But with normal run this benchmark hangs after all tests:

which node version?

@vsemozhetbyt
Copy link
Contributor

@refack I have flaky results with almost all versions:

4.8.2: OK / hung after all tests
6.10.2: OK / hung after dur=5 type="asc" len=16777216 / hung after all tests
7.8.0: OK
8.0.0: OK / hung after dur=5 type="utf" len=16777216: / hung after all tests

When it hangs, there are 2 node processes. When I type Ctrl+c, one process is aborted. second remains and I can't even kill it via process manager (Access Denied). I've run recently almost all Node benchmarks and had no such problem with any. Can anybody reproduce?

@refack
Copy link
Contributor Author

refack commented Apr 6, 2017

Can anybody reproduce?

Windows 7 SP1 x64?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 6, 2017

Windows 7 SP1 x64

Yep.

@refack
Copy link
Contributor Author

refack commented Apr 6, 2017

@vsemozhetbyt try this trick: rename the node.exe you're testing, then see if the hung node.exe is the tested node or the one run by autocannon
(always good to use procmon run as administrator, will also give you the command line a process was run with)
image

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 6, 2017

@refack Yes, I've already have a set of renamed Nodes and do usualy use procmon)

1: Total Commander with nodes.
2: cmd.exe with one succesful and one hung run.
3: procmon: a tooltip for the second process. Ctrl+c abort the first process, the second remains hung and unkillable.
1

@refack
Copy link
Contributor Author

refack commented Apr 6, 2017

@vsemozhetbyt ups for the setup 🙆

I can't repro...
The "unkillable" issue makes me think you should check the unholy trinity "MpsSvc, WSearch, WinDefend" or any other firewall/vpn/anti-virus

@vsemozhetbyt
Copy link
Contributor

"MpsSvc, WSearch, WinDefend" — not found / disabled)

Well, I think we can ignore this my issue as too flaky. Thank you again for fixing!

@refack
Copy link
Contributor Author

refack commented Apr 6, 2017

I think we can ignore this my issue as too flaky. Thank you again for fixing!

Maybe a good restart...
Anyway, have a nice one!

@refack refack force-pushed the fix11972 branch 3 times, most recently from e5c731b to f9ae053 Compare April 7, 2017 03:11
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@refack refack self-assigned this Apr 8, 2017
@joyeecheung joyeecheung added the windows Issues and PRs related to the Windows platform. label Apr 11, 2017
fixes nodejs#11972

PR-URL: nodejs#12258
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack refack merged this pull request into nodejs:master Apr 14, 2017
@refack
Copy link
Contributor Author

refack commented Apr 14, 2017

Landed in 16073c083e
Landed in: fbe946b

@refack refack deleted the fix11972 branch April 14, 2017 04:08
@vsemozhetbyt
Copy link
Contributor

@refack Please, don't forget about commit message guidelines :)

@watilde
Copy link
Member

watilde commented Apr 14, 2017

This command may support you when you want to check the commit message :)

@refack
Copy link
Contributor Author

refack commented Apr 14, 2017

This command may support you when you want to check the commit message :)

https://github.com/evanlucas/core-validate-commit

I'll need to find a way to hook it into git for myself, in just the right conditions.

evanlucas pushed a commit that referenced this pull request 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 evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request 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 pull request May 2, 2017
fixes #11972

PR-URL: #12258
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack refack removed their assignment Jun 12, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit that referenced this pull request 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
Copy link
Member

gibfahn commented Jun 18, 2017

Landed fbe946b on v6.x, LMK if this is incorrect.

@refack
Copy link
Contributor Author

refack commented Jun 18, 2017

🤔 it's a bug fix in /benchmark/ so it makes sense to except it from the "no automatic benchmarking porting" rule.

gibfahn pushed a commit that referenced this pull request 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 pull request Jul 11, 2017
fixes #11972

PR-URL: #12258
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
@amark
Copy link

amark commented Sep 26, 2020

@refack do you by chance know what is going on here? readStart 23 seconds to complete, blocking all other NodeJS operations. IDK if I should open an issue, tho this is v12.14.1 so I wouldn't until I test against latest stable & 14.

Here's a screenshot of the time-ordered flamegraph as rendered by speedscope:

Screen Shot 2020-09-26 at 2 24 32 AM

It caused a in-production server to peg past 100% CPU utilization & drop 17K concurrent websocket connections (I presume from timeout). It regularly seems to occur at ~17.4K+ concurrency, not randomly, in low memory with otherwise ~27% CPU usage.

Thanks, just looking for pointers or if anybody has seen something similar.

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. net Issues and PRs related to the net subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

benchmark: net/tcp-raw-pipe.js gives wrong data
10 participants