Page MenuHomePhabricator

Statistic/Timer: Include timers in PrintStatisticsJSON().
ClosedPublic

Authored by MatzeB on Oct 13 2016, 7:01 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 74609.Oct 13 2016, 7:01 PM
MatzeB retitled this revision from to Statistic/Timer: Include timers in PrintStatisticsJSON()..
MatzeB updated this object.
MatzeB added reviewers: bruno, vsk, sebpop, mehdi_amini.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.

Why are we using JSON for this? Should we use YAML for consistency with other things we serialize to text?

Why are we using JSON for this? Should we use YAML for consistency with other things we serialize to text?

Technically YAML is a superset of JSON so this is YAML. Though admittedly I just looked that up, this really was a "I don't care" decision, my first version used in http://reviews.llvm.org/D20995 used CSV but the reviewers rather liked JSON.

Maybe I can sneak the escape function into one of our yaml headers then. Though I probably don't want to use all the IO machinery in YAMLTraits.h just to output some key/value pairs...

MatzeB updated this revision to Diff 74745.Oct 14 2016, 3:16 PM

Use yaml::needsQuotes() instead of adding a new JSONEscape() function/JSON.h header.

bruno edited edge metadata.Oct 14 2016, 7:07 PM

This is very nice. A couple of questions/comments:

  • Looks like if you pass -stats -stats-json -time-passes then all timers and statistics are printed together, what happens if you just pass -stats-json -time-passes, would that only output the timers in json format? Also, when printed together, are they part of the same "group"? it's not clear from the tests how the output layout works.
  • Do you plan to somehow track different runs of the same pass? Right now that could be inferred by looking into the output order, but it would be nice (not necessarily in this patch) to easily access that information; I remember it to be really useful when comparing compile time between two different compilers, I could then notice when one compiler run the same pass more times than the other and it would also help identify when specific runs were slower due to the influence of other opts.

This is very nice. A couple of questions/comments:

  • Looks like if you pass -stats -stats-json -time-passes then all timers and statistics are printed together, what happens if you just pass -stats-json -time-passes, would that only output the timers in json format? Also, when printed together, are they part of the same "group"? it's not clear from the tests how the output layout works.

The way this currently works is that the statistics printing code grabs the timers when printing (as a side effect this clears the timers so they won't get printed again by the timer printing code). This means that with "-stats-json -time-passes" without "-stats" you will get no stats printed and the timers will end up being printed in the traditional format. I don't think we need such a mode either as I expect the data to be processed by scripts which could just as well do the filtering afterwards.

The current output format is simply a list of key/values. However I introduced some hierarchy in the key names, so you will see key names like "mem2reg.NumSingleStore", "time.regalloc.seed.user" or "time.pass.Scalar Evolution Analysis.wall" so you can filter somewhat by prefixes/suffixes of the keys.

  • Do you plan to somehow track different runs of the same pass? Right now that could be inferred by looking into the output order, but it would be nice (not necessarily in this patch) to easily access that information; I remember it to be really useful when comparing compile time between two different compilers, I could then notice when one compiler run the same pass more times than the other and it would also help identify when specific runs were slower due to the influence of other opts.

This patch does not change the way timers are run, so all timers with the same name and therefore all runs of a pass with the same name are merged together. It would indeed be nice to introduce a model here where rather dump some form statistic events rather than the summed stats; But that is a more fundamental change that needs to be done in a different commit. I'd be happy to help discuss, design and review if someone wants to attack this.

bruno accepted this revision.Nov 17 2016, 3:02 PM
bruno edited edge metadata.

Thanks for the detailed explanations Matthias. LGTM

This revision is now accepted and ready to land.Nov 17 2016, 3:02 PM
This revision was automatically updated to reflect the committed changes.