nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.39k stars 29.51k forks source link

Benchmark CI is using the test-double benchmarker for some HTTP benchmarks #16557

Closed joyeecheung closed 6 years ago

joyeecheung commented 6 years ago

See https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/22/console Refs: https://github.com/nodejs/node/pull/15752

joyeecheung commented 6 years ago

I am not sure why but the CI is using the test-double benchmarker to run some HTTP benchmarks...does the CI machine have wrk or autocannon setup? @nodejs/build

tail -n +2 out.csv | grep test-double | cut -d , -f 2 | sort | uniq
 "http/chunked.js"
 "http/cluster.js"
 "http/end-vs-write-end.js"
 "http/simple.js"
refack commented 6 years ago

So the machine does not have node nor npm installed... There are a few options: 1) Install (but who updates?) 2) Add to https://github.com/nodejs/benchmarking/blob/master/experimental/benchmarks/community-benchmark/run.sh (and refresh on server) 3) Add to Jenkins job

joyeecheung commented 6 years ago

@refack

I think the easiest & quickest way is to go ahead an install wrk there. I can update it from time to time if I just need to ssh there and do this (IIUC? I guess all I need to do is sudo apt-get install wrk for now and probably need to build it from source if important updates are not available from apt), although I don't expect a lot of important updates of wrk.

Safest solution would probably be using autocannon installed with the Node.js binary/npm that it has built/pulled, if the network config allows that.

Not quite sure what 3. Add to Jenkins job means?

AndreasMadsen commented 6 years ago

I would prefer wrk. It has lower variance so it will give us better results. It will also be easier to compare runs because wrk doesn't depend on node.

mhdawson commented 6 years ago

We really should have ansible scripts that configure the 2 new benchmark machines. Manual installs will only persist until we have to re-install the machines.

If that is not possible, at the very least we should have a PR adding documentation to the benchmark repo that documents what needs to be manually installed for the the job in question.

mhdawson commented 6 years ago

In cases of installation of npm's (non-global) I have done that install in the job itself. For things that need to be installed with apt-get I don't think they should be in the jobs as they change the machine configuration.

refack commented 6 years ago

I think the easiest & quickest way is to go ahead an install wrk there. I can update it from time to time if I just need to ssh there and do this (IIUC? I guess all I need to do is sudo apt-get install wrk for now and probably need to build it from source if important updates are not available from apt), although I don't expect a lot of important updates of wrk.

The ideal is for the jobs to be self-sufficient. Since node, npm and wrk can be installed without apt IMHO the installation should go in https://github.com/nodejs/benchmarking/blob/master/experimental/benchmarks/community-benchmark/run.sh (this is what the job runs) Even better is to migrate to a pipeline similar to https://github.com/nodejs/build/pull/956

mhdawson commented 6 years ago

@refack either of those 2 suggestions sound good to me.

gareth-ellis commented 6 years ago

Hello,

I don't like the idea of having a static copy of node on the machine, as some jobs add node to the path, and then run tests - if we have another node on the path, we could risk running the wrong one (if for example the node we've just built failed, and wasn't picked up; jobs would keep running but on the static version of node).

I think either adding to an ansible script so its installed globally, or we can include it in the package. I've just had a go at building on a local machine, and it takes a couple of minutes to build there - so I think what might be best is to add to an ansible script to checkout a specific version, build it, and then we could add it to the same location we have jmeter etc, and the benchmarking run.sh can add wrk to the path for its runs.

Does that sound like an idea?

joyeecheung commented 6 years ago

@gareth-ellis Sorry I am not quite following, I think adding wrk to the machine does not require a static copy of node? Or am I missing something?

joyeecheung commented 6 years ago

@gareth-ellis Oh is it for installing autocannon? I think if we choose wrk instead (as suggested in https://github.com/nodejs/node/issues/16557#issuecomment-340458380) we would not have to worry about having a node in the path at all. Personally I prefer wrk as well.

(If it comes from https://github.com/nodejs/node/issues/16557#issuecomment-340188290, I meant installing autocannon using one of the node built for benchmarking, probably the master one, not installing a separate copy of node just for autocannon)

gareth-ellis commented 6 years ago

Yes, sorry. It seemed the conversation was suggesting autocannon which would/may require a static node installed, or wrk which does not. I prefer the idea of not having static copies of node around, so wrk seems a good way forward.

mhdawson commented 6 years ago

We should add a comment to this issue https://github.com/nodejs/build/issues/868 if we install wrk manually on the machines

gareth-ellis commented 6 years ago

Is there ansible scripts for the benchmarking machine already? Seems would be quite an easy thing to write in ansible, so if there's somewhere I can add the bits required to install wrk via ansible, I'm happy to submit a PR

mhdawson commented 6 years ago

@gareth-ellis unfortunately we don't have an ansible script for the benchmarking machines yet. See discussion here: https://github.com/nodejs/build/issues/868

AndreasMadsen commented 6 years ago

Could you guys just install wrk for now? It is super annoying that we can't run http benchmarks.

Regarding the update issue, we have the same issue for the R packages so I don't see what is new. I respect the need to automate this, but this is taking too long. The last wrk update was almost a year ago, so I think we can survive no automated updates in the meantime.

mhdawson commented 6 years ago

I manually installed wrk on the 2 machines. We really need to make some progress on the automation side but since we've not managed to do it for too long I went ahead and did the install.

AndreasMadsen commented 6 years ago

Thanks :)

joyeecheung commented 6 years ago

I think this can be closed now. Ideally the benchmark jobs should be migrated to ansible but that should be an issue in the build repo.