The Timer destructor would grab a global mutex in order to update execution time. Add a class to define a category once, statically; the class adds itself to an atomic singly linked list, and thus subsequent updates only need to use an atomic rather than grab a lock and perform a hashtable lookup.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Don't forget to update the usages in unit tests (and make sure the check-lldb-unit target passes).
Seems reasonable, however: I am not sure who actually uses these timers. I'd be tempted to just remove the timers that are causing the contention.
include/lldb/Core/Timer.h | ||
---|---|---|
26 ↗ | (On Diff #97708) | I would put this inside the Timer class, so that we can refer to it as Timer::Category. I guess tastes might differ. |
source/Core/Timer.cpp | ||
147 ↗ | (On Diff #97708) | This could be a range-based loop. |
source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp | ||
5421 ↗ | (On Diff #97708) | I am not sure how you format your changes, but you should make sure you format only the lines you've touched, and not the whole files. git-clang-format https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format will do that for you -- when you set it up, you just run git clang-format HEAD^ and it will format your last patch). |
IMO we should either keep the timers and make them fast, or get rid of all timers. I don't like the idea of training developers to know that timers are slow, and should be used sparingly. Especially since the fix is relatively simple.
Though an atomic skip list would be another way to improve performance without requiring changing each callsite; the downside is the lg-n lookup when tallying the time.
source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp | ||
---|---|---|
5421 ↗ | (On Diff #97708) | Yeah that'll be helpful. I have default .emacs settings for work that don't necessarily apply well to the llvm codebase, this tool will make it easier to fix up. |
Ok, then let's keep them. I don't mind changing all call sites -- having a separate category object is the cleanest solution with least magic. However see my comments about category naming and merging.
unittests/Core/TimerTest.cpp | ||
---|---|---|
39 ↗ | (On Diff #97880) | 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.) |
48 ↗ | (On Diff #97880) | ss.GetStringRef().count("CAT1") == 1 |
remove same-name-different-site support from Timer::Category
unittests/Core/TimerTest.cpp | ||
---|---|---|
39 ↗ | (On Diff #97880) | I don't think so. git grep Timer::Category | grep -v LLVM_PRETTY_FUNCTION The test might be testing for recursion-safe timers, in which case I can just change the declaration of t2: so it uses the same category object. |
lgtm, thank you.
unittests/Core/TimerTest.cpp | ||
---|---|---|
39 ↗ | (On Diff #97880) | Yes, the test was testing recursive timers (I wrote it). |