[Lldb-commits] [PATCH] D32823: Remove an expensive lock from Timer

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

lgtm, thank you.



================
Comment at: unittests/Core/TimerTest.cpp:39
     std::this_thread::sleep_for(std::chrono::milliseconds(10));
-    Timer t2("CAT1", "");
+    // Explicitly testing the same category as above.
+    static Timer::Category tcat1b("CAT1");
----------------
scott.smith wrote:
> labath wrote:
> > Do we actually need to support that? Since we already have a nice Category object then I think we should use that as a unique key for everything (including printing). There no reason why the category needs to have local scope. If it really needs to be used from multiple places, we can expose the object at a higher scope. For the tests, this could be done by having a fixture which creates the categories in the SetUpTestCase static function. Alternatively, we could have Category object be based on llvm::ManagedStatic, so it would go away when you call llvm_shutdown, which would enable us to have truly isolated and leak-free tests. (Right now we don't call llvm_shutdown in the main app as we cannot shutdown cleanly, but it would be great if one day we could.)
> I don't think so.
> 
> git grep Timer::Category | grep -v LLVM_PRETTY_FUNCTION
> shows one place (other than the unit test) where the name is explicitly set, and that's because that function already has its own scoped timer.  All the other places are unique (one per function).
> 
> The test might be testing for recursion-safe timers, in which case I can just change the declaration of t2:
> Timer t2(tcat1a, "")
> 
> so it uses the same category object.
Yes, the test was testing recursive timers (I wrote it).


Repository:
  rL LLVM

https://reviews.llvm.org/D32823