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

[Scott Smith via Phabricator via lldb-commits]

scott.smith added inline comments.


================
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");
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D32823