Page MenuHomePhabricator

Remove an expensive lock from Timer

Authored by scott.smith on May 3 2017, 12:34 PM.



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.

Diff Detail


Event Timeline

scott.smith created this revision.May 3 2017, 12:34 PM
labath edited edge metadata.May 4 2017, 2:35 AM

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.

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.

147 ↗(On Diff #97708)

This could be a range-based loop.

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 will do that for you -- when you set it up, you just run git clang-format HEAD^ and it will format your last patch).

scott.smith marked 3 inline comments as done.May 4 2017, 2:12 PM

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.

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.

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.

scott.smith updated this revision to Diff 97880.May 4 2017, 2:41 PM
scott.smith marked an inline comment as done.

updated per review comments.
added a test to fix the Jurassic Park problem.

labath added a comment.May 5 2017, 2:32 AM

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.

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

scott.smith updated this revision to Diff 97973.May 5 2017, 9:36 AM
scott.smith marked 2 inline comments as done.

remove same-name-different-site support from Timer::Category

39 ↗(On Diff #97880)

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.

labath accepted this revision.May 5 2017, 10:09 AM

lgtm, thank you.

39 ↗(On Diff #97880)

Yes, the test was testing recursive timers (I wrote it).

This revision is now accepted and ready to land.May 5 2017, 10:09 AM

Can someone please commit this? Thank you!

This revision was automatically updated to reflect the committed changes.