[Lldb-commits] [PATCH] D61235: Add more information to the log timer dump

[António Afonso via Phabricator via lldb-commits]

aadsm planned changes to this revision.
aadsm marked 2 inline comments as done.
aadsm added inline comments.


================
Comment at: lldb/source/Utility/Timer.cpp:110-123
+namespace {
+struct Stats {
+  uint64_t nanos;
+  uint64_t nanos_total;
+  uint64_t nanos_child;
+  uint64_t count;
+};
----------------
labath wrote:
> You can just move the `const char *` into the `Stats` struct and get rid of the std::pair. Then the `CategoryMapIteratorSortCriterion` thingy can also become a regular operator<.
ah yeah, that will be much better.


================
Comment at: lldb/unittests/Utility/TimerTest.cpp:89
+  // 0.102982273 sec (total: 0.126s; child: 0.023s; count: 1) for CAT1
+  // 0.023058228 sec (total: 0.036s; child: 0.012s; count: 2) for CAT2
+  StreamString ss;
----------------
labath wrote:
> I'm not sure this second line is really helpful here. Looking at this output, I would have never guessed that these numbers mean we were running two recursive timers belonging to the same category, and the entire thing finished in 20ms.
> 
> Maybe this isn't a real usage problem, as normally we don't have recursive timers (I believe), but in that case, we probably shouldn't have them in this test either (as right now it is the only documentation about how these times are supposed to work). 
> 
> What's the reason for using recursive timers here? If you just wanted to check that the invocation count is 2, then you can wrap the two timers in `{}` blocks, so that they execute "sequentially" instead of recursively. (Which I guess would match the typical scenario where a timer-enabled function calls another timer-enabled function two times in a loop.)
you're totally right, I actually didn't think much about it when I wrote the test but this should definitely be sequential not recursive.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61235/new/

https://reviews.llvm.org/D61235