This is an archive of the discontinued LLVM Phabricator instance.

[Support] TimerGroup changes
AbandonedPublic

Authored by lebedev.ri on May 8 2018, 2:44 PM.

Details

Summary

This is needed for the continuation of D46504,
to be able to store the timings.

The floating-point values are dumped with no precision loss.

See dependent differential D46602 for details/use-case.

Diff Detail

Repository
rL LLVM

Event Timeline

@lebedev.ri LLVM already has facilities for outputting statistics in JSON, it seems that would be sufficient for your purposes?
I did something similar for the static analyzer in https://reviews.llvm.org/D43131

@lebedev.ri LLVM already has facilities for outputting statistics in JSON, it seems that would be sufficient for your purposes?
I did something similar for the static analyzer in https://reviews.llvm.org/D43131

YAML != JSON.
I did see all that TimerGroup stuff.
I will take a second look, but the dumpVal() change will still be needed either way.

lebedev.ri planned changes to this revision.May 9 2018, 5:32 AM
lebedev.ri updated this revision to Diff 145900.May 9 2018, 5:41 AM
lebedev.ri retitled this revision from [Support] Print TimeRecord as CSV to [Support] TimerGroup changes.
lebedev.ri edited the summary of this revision. (Show Details)
lebedev.ri added a reviewer: NoQ.
lebedev.ri added subscribers: xazax.hun, szepet, a.sidorin.
  1. Drop CSV stuff
  2. Add constructor from previously-collected StringMap<TimeRecord>
  3. Make printJSONValues() public.
  4. Dump floating-point values in scientific format with full precision.
lebedev.ri updated this revision to Diff 145903.May 9 2018, 5:59 AM

Add lock to now-public printJSONValues().

I have no understanding of the static TimerGroupList,
but this is symmetrical with other [public] TimerGroup::*print* methods.

alexfh added a comment.May 9 2018, 7:44 AM

I wonder why JSON? What kind of a tool do folks use to process the outputs of printJSONValue? Is there anything existing or is JSON used "just in case"? I personally use either spreadsheets, python or shell when I deal with this kind of data, and all three of them can work reasonably well with CSV (plus the data is pretty simple).

lebedev.ri updated this revision to Diff 145994.May 9 2018, 1:11 PM
  • Use sane (not the same as the one right before..) suffix for when json-printing mem usage. Admittedly, i haven't tried it, but it just does not make sense otherwise.

Ping.
Any thoughts on these Timer changes?

I see four separate changes: s/.sys/mem one (can be committed without review), exposing printJSONValues and consequent adding of a lock, adding a constructor accepting a map, and fixing formatting in printJSONValue. All four look good to me, but probably should be reviewed separately.

lib/Support/Timer.cpp
385

This change seems unrelated to the new constructor accepting map, right?

405

That's independent from this change, right?

lebedev.ri abandoned this revision.May 16 2018, 4:59 AM

I see four separate changes: s/.sys/mem one (can be committed without review), exposing printJSONValues and consequent adding of a lock, adding a constructor accepting a map, and fixing formatting in printJSONValue. All four look good to me, but probably should be reviewed separately.

Thank you for taking a look!
Posted as D46936, D46937, D46938, D46939

I see four separate changes: s/.sys/mem one (can be committed without review), exposing printJSONValues and consequent adding of a lock, adding a constructor accepting a map, and fixing formatting in printJSONValue. All four look good to me, but probably should be reviewed separately.

Thank you for taking a look!
Posted as D46936, D46937, D46938, D46939

@alexfh & @george.karpenkov Thanks for reviewing those!
Not sure yet whether i will land them right away, or wait for clang-tidy part.

Not sure yet whether i will land them right away, or wait for clang-tidy part.

I think it's better to land, as otherwise you risk merge conflicts, and implicit conflicts (git reports no errors, but the resulting code is bad) can be very annoying.