This is an archive of the discontinued LLVM Phabricator instance.

Add a script to run various benchmarks and send the result to lnt
ClosedPublic

Authored by rafael on Nov 1 2017, 1:20 PM.

Details

Summary

Lnt is both a server and a set of script for benchmarking llvm.

I don't think it makes sense to use the scripts for lld since our benchmarks are quite different.

The server on the other hand is very general and seems to work well for tracking any quantities.

This patch adds a script to lld that can be used to run various benchmarks and send the result to lnt.

The benchmarks are assumed to each be a response file in a subdirectory. Each subdirectory can contain multiple response files. That can be used to have a plain response.txt and a response-icf.txt for example. The name of each benchmark is the combination of the directory name and the "flavor": firefox-gc, chromium-icf, etc.

For the first version the script uses perf and collects all the metrics that a plain "perf stat" prints.

This script can then be used by a developer to test a patch or by a bot to keep track of lld's performance.

Diff Detail

Event Timeline

rafael created this revision.Nov 1 2017, 1:20 PM
MatzeB edited edge metadata.Nov 1 2017, 1:51 PM

Lnt is both a server and a set of script for benchmarking llvm.

I don't think it makes sense to use the scripts for lld since our benchmarks are quite different.

Indeed and as far as I am concerned, I'd like to see the "benchmark running" part of LNT go away and be replaced by scripts that ship with the test suites even for the existing suites.

utils/benchmark.py
28

Looks like you create a new schema. It's probably best if you put the schema file next to the benchmark/submission script.

rafael updated this revision to Diff 121215.Nov 1 2017, 5:35 PM

Add schema.

Make it more python3 friendly

grimar added a subscriber: grimar.Nov 1 2017, 11:30 PM
jhenderson edited edge metadata.Nov 2 2017, 3:10 AM

Overall approach looks reasonable to me. There's clearly some work to do to get this compatible with Windows (we'd need suitable replacements for cset and perf), but I don't think it should be too difficult. That being said, I don't know if there's a way to get the same set of information on Windows or not. It might be that we have to use a different schema in that case, as well as a different parser for the results. Overall, the code is small enough that it shouldn't be too hard to do this later though.

utils/benchmark.py
24

Why is this a configurable option, but perf not? Is the intent to be able to point to a specific cset executable, or to be able to swap it out for a different executable?

27

Could we have some help text here, please saying that this is the LNT server URL.

71

Nit: extra unnecessary leading space inside brackets.

86

Could you comment what this run is for, please?

(I assume that it is to warm up the file cache or similar, but it'd be good to have this confirmed).

Also, this isn't in the try/except block the later run is. Any particular reason?

104

I know that this would lead to less stable metrics, but it might be nice to have the "--no-threads" on a switch, probably defaulting to no threads. That way, if people want to compare their performance with and without threading, they can.

117

This doesn't look right - why are we setting the start and end time to the same value?

rafael updated this revision to Diff 121419.Nov 2 2017, 5:29 PM

address review comments.

Ok, the script looks fine to me. My one hesitation is the way the cset stuff works - it's pretty inflexible currently, but it gave me an idea. As I mentioned separately in the email thread, this feels like one of the "wrapper" programs I mentioned. Indeed, perf itself is as well, although I accept that this particular case is different, since the script needs to parse the output to store in the database.

To make it more general purpose, how about the following? We replace the "cset" switch with something like "wrapper". The switch then takes a list of strings that are a wrapper program and its arguments. The current behaviour would then be emulated by something like "--wrapper=cset,shield,-e" or whatever syntax is easiest (and this could be the default to reduce the amount of typing on Linux). More than one --wrapper could be specified and they'd be daisy-chained together, so something like "--wrapper=cset,shield,-e --wrapper=something,-somewhere=blah,-somehow" would lead to python running "sudo cset shield -e something -somewhere=blah -somehow perf -- stat ...". This would allow users to not use cset, if they wanted, and/or use other wrapper programs to configure other aspects of the LLD runtime environment.

How does that sound?

I really liked the idea of the --wrapper command line.

With that I can run it on my machine with

--wrapper sudo,/home/espindola/inst/cset/bin/cset,shield,-e

I just tried running this locally without using cset, and I got an error from the '--' part of the command, which presumably is intended for use with cset. Removing it would cause cset to fail, if used as you described, but it looks like it is possible to do "--wrapper sudo,cset,shield,-e,--" and still get the expected result, even though the -e is in the seemingly wrong place. Note: I have not verified that in this case the run is definitely shielded, although it looks likely.

One feature request, that doesn't need doing in the first iteration, is for the ability to print the summarised results to stdout optionally instead of posting to LNT. At the moment, this script requires LNT to run, which for a casual user who just wants to quickly compare two sets of times may not be the best situation.

Remove hard coded --. It should be passed by --wrapper.

Handle an empty --wrapper.

jhenderson accepted this revision.Nov 14 2017, 2:18 AM

Thanks. LGTM, though it might be good to get a second opinion.

I assume that this is being committed into lld/utils, rather than llvm/utils?

This revision is now accepted and ready to land.Nov 14 2017, 2:18 AM
espindola closed this revision.Mar 14 2018, 3:38 PM
espindola added a subscriber: espindola.

318158