Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign uptest: add test for Linux perf #20783
Conversation
nodejs-github-bot
added
the
test
label
May 16, 2018
This was referenced May 16, 2018
mmarchini
force-pushed the
mmarchini:test-linux-perf
branch
from
d05a2ac
to
34e1c60
May 16, 2018
mmarchini
added
dont-land-on-v4.x
dont-land-on-v6.x
dont-land-on-v8.x
and removed
dont-land-on-v4.x
labels
May 16, 2018
This comment has been minimized.
This comment has been minimized.
|
Very good job! Do you know what is the overhead of the flag? |
This comment has been minimized.
This comment has been minimized.
|
This actually looks more like a V8 test, although I understand that it may be more difficult to detect all the dependencies and launch perf in the V8 test suites. |
This comment has been minimized.
This comment has been minimized.
Not yet, but it should be very low (imperceptible for most applications). Most overhead will be when functions are created, and even then it should be below 10%. I want to run benchmarks to measure that in the next few days.
There's a test for the flag on V8, but there's no test specifically for Linux perf there. Since Linux perf is not officially supported by V8 I don't think we would be able to test it there. This test is similar to |
mhdawson
reviewed
May 17, 2018
| @@ -7,6 +7,8 @@ prefix parallel | |||
| [true] # This section applies to all platforms | |||
| # Postmortem debugging data is prone to accidental removal during V8 updates. | |||
| test-postmortem-metadata: PASS,FLAKY | |||
| # Linux perf is prone to breakage during V8 updates. | |||
| test-linux-perf: PASS,FLAKY | |||
This comment has been minimized.
This comment has been minimized.
mhdawson
May 17, 2018
Member
Is this the case here? I'm a bit worried that it will make it less likely to be caught early on. I'm not sure it is as likely to be broken as the postmortem data to warrant this.
This comment has been minimized.
This comment has been minimized.
mmarchini
May 18, 2018
Author
Member
We do know if it breaks on nodejs/node-v8, that's how we know when there are postmortem metadata changes today.
Removing flakiness status from this test would make it a blocker on V8 updates if Linux perf stops to work again. I can remove it if that's what we want.
This comment has been minimized.
This comment has been minimized.
mmarchini
May 21, 2018
Author
Member
Removed flakiness status from this test. It's easy to add it back if we want to.
This comment has been minimized.
This comment has been minimized.
mhdawson
May 23, 2018
Member
@mmarchini seems like the right answer, we can always change if it is a problem.
mmarchini
added
the
work in progress (WIP)
label
May 21, 2018
This comment has been minimized.
This comment has been minimized.
cjihrig
reviewed
May 23, 2018
| // NOTE (mmarchini): this test is meant to run only on Linux machines with Linux | ||
| // perf installed. It will skip if those criteria are not met. | ||
|
|
||
| const common = require('../common'); |
This comment has been minimized.
This comment has been minimized.
cjihrig
May 23, 2018
Contributor
Should there be a check for common.hasCrypto due to its use in the fixture?
This comment has been minimized.
This comment has been minimized.
|
From nodejs/build#1274:
We could move those tests to My only concern with this approach is that those tests won't run on cherry-pick PRs... Maybe they could run under |
This comment has been minimized.
This comment has been minimized.
|
/cc @nodejs/v8-update this might be of your interest |
mmarchini
force-pushed the
mmarchini:test-linux-perf
branch
from
d8c5eaf
to
87446ce
May 28, 2018
This comment has been minimized.
This comment has been minimized.
|
Moved test-postmortem-metadata and test-linux-perf to We can run @joyeecheung @mhdawson @rvagg PTAL |
This comment has been minimized.
This comment has been minimized.
|
@rvagg did you get perf installed on some machines? If so will look at how we add this to the v8 test job. |
This comment has been minimized.
This comment has been minimized.
|
ping @rvagg |

mmarchini commentedMay 16, 2018
•
edited
Linux perf has been broken on V8 since the Turbofan/Ignition pipeline became the default compiler. Recently on V8 6.7, we got it back to work (through a flag). Since there are some Node.js tools and some huge Node.js deployments relying on Linux perf (and other external profilers), having tests will help to keep those tools more stable. This first version is marked as flaky since it shouldn't block releases or PRs, but it will help us know if something breaks in advance through v8-canary.
There are still some things to do before landing this, but I wanted to start gathering some feedback.
TODO list:
/cc @nodejs/diagnostics
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes