[Support] TimerGroup changes
AbandonedPublic

Authored by lebedev.ri on Tue, May 8, 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

@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.Wed, May 9, 5:32 AM
lebedev.ri updated this revision to Diff 145900.Wed, May 9, 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.Wed, May 9, 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.Wed, May 9, 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.Wed, May 9, 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.Wed, May 16, 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.