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 CI is using the test-double benchmarker for some HTTP benchmarks #16557

Closed
joyeecheung opened this issue Oct 27, 2017 · 19 comments
Closed
Assignees
Labels
benchmark Issues and PRs related to the benchmark subsystem.

Comments

@joyeecheung
Copy link
Member

  • Version: master

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

@joyeecheung joyeecheung added the benchmark Issues and PRs related to the benchmark subsystem. label Oct 27, 2017
@joyeecheung
Copy link
Member Author

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
Copy link
Contributor

refack commented Oct 27, 2017

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
Copy link
Member Author

joyeecheung commented Oct 28, 2017

@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?

@joyeecheung joyeecheung self-assigned this Oct 28, 2017
@AndreasMadsen
Copy link
Member

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
Copy link
Member

mhdawson commented Nov 1, 2017

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
Copy link
Member

mhdawson commented Nov 1, 2017

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
Copy link
Contributor

refack commented Nov 1, 2017

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 nodejs/build#956

@mhdawson
Copy link
Member

mhdawson commented Nov 3, 2017

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

@gareth-ellis
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member Author

joyeecheung commented Nov 28, 2017

@gareth-ellis Oh is it for installing autocannon? I think if we choose wrk instead (as suggested in #16557 (comment)) 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 #16557 (comment), 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
Copy link
Member

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
Copy link
Member

We should add a comment to this issue nodejs/build#868 if we install wrk manually on the machines

@gareth-ellis
Copy link
Member

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
Copy link
Member

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

@AndreasMadsen
Copy link
Member

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
Copy link
Member

mhdawson commented Jan 5, 2018

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
Copy link
Member

Thanks :)

@joyeecheung
Copy link
Member Author

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.

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.
Projects
None yet
Development

No branches or pull requests

5 participants