This is an archive of the discontinued LLVM Phabricator instance.

[Support] Add reentrant start/stop Timer methods
AbandonedPublic

Authored by modocache on Aug 17 2017, 1:58 PM.

Details

Summary

In Clang's lib/CodeGen/CodeGenAction.cpp, a timer is used to measure
the amount of time spent generating LLVM IR. The timer is started and
stopped in a method, BackendConsumer::HandleTopLevelDecl, that is called
recursively. llvm::Timer does not allow starting a timer twice, so to
prevent a runtime assertion that would occur if the timer is started
a second time as the method recurses, it's guarded by a "reference
count" variable: if the reference count is incremented from 0 to 1, the
timer is started. Subsequent calls to start the timer increment the
reference count, but do not call startTimer(). stopTimer() is called when
the reference count goes from 1 to 0.

This pattern is useful when timing any recursive method, such as in
https://reviews.llvm.org/D36492.

Add startReentrantTimer and stopReentrantTimer methods that can be used for
situations such as these. Unlike startTimer/stopTimer, multiple calls to
startReentrantTimer are fine, as long as stopReentrantTimer is called an
equal number of times.

Event Timeline

modocache created this revision.Aug 17 2017, 1:58 PM
vsk edited edge metadata.Aug 17 2017, 2:13 PM
vsk added subscribers: davide, rafael.

I tried this a while back: https://reviews.llvm.org/D20735

I ended up abandoning the patch because some some folks weren't totally comfortable with a timer that can be started twice. I think we're seeing that this is a use case that's becoming more important. Actually, @rafael mentioned that he's OK with ref-counted timers "if it is cumbersome to use a non ref counted timer in the current users", and it looks like that's the case. @davide, wdyt of revisiting the ref-counted timer decision?

Friendly ping -- does anyone have any thoughts here?

davide added a subscriber: rsmith.Aug 21 2017, 10:07 AM

Personally, I'd still be interested in understanding whether this can be fixed in the consumers.
You probably want to hear what @rsmith thinks (i.e. whether it's feasible to fix this in clang, which seems to be the only place where this code would be used).
In case you can't, I'm OK with revisiting/allowing this feature in Support/.

MatzeB edited edge metadata.Aug 21 2017, 10:27 AM

For something as simple as a timer, having two std::string, 3 pointers and now a vtable feels like too much. Of course that's not the fault of this patch.

That said you are adding 1 pointer (for the vtable) to the timer class and the start/stop calls become virtual. May just as well add the counter to the baseclass instead and have it always refcount to avoid the virtual methods.

Or alternatively just add startReentrant()/stopReentrant() methods to the baseclass. So the default startTimer()/stopTimer() methods remain the same?

Friendly ping -- does anyone have any thoughts here?

modocache planned changes to this revision.Aug 22 2017, 7:22 AM

Thanks for the suggestions, @MatzeB! I'll try to get rid of the added vtable.

fix this in clang, which seems to be the only place where this code would be used

@davide That's true, for now. But I also have https://reviews.llvm.org/D36492 out, which can also make use of this. So I am anticipating this will be used in more than just one place, and that there will be cases where it could not reasonably be solved in consumers.

modocache updated this revision to Diff 112182.Aug 22 2017, 8:40 AM
modocache retitled this revision from [Support] Add "reference-counted" Timer to [Support] Add reentrant start/stop Timer methods.
modocache edited the summary of this revision. (Show Details)

Instead of using virtual methods, add Timer::startReentrantTimer and Timer::stopReentrantTimer. @MatzeB, let me know if this isn't what you had in mind -- thanks!

One downside of separate startReentrantTimer and stopReentrantTimer methods is that we may need to add a bool IsReentrant parameter to the initializer of TimeRegion, in order to support the way it's used in https://reviews.llvm.org/D36946.

FWIF: Looking at https://reviews.llvm.org/D36946 I'm not convinced yet that timer is a good idea there at all.

include/llvm/Support/Timer.h
82

Maybe this should be called StartCount instead of RefCount? It should be possible to get rid of the Running member at that point and check StartCount > 0 instead.

modocache updated this revision to Diff 112371.Aug 23 2017, 8:09 AM

Replace Running member with StartCount.

Looking at https://reviews.llvm.org/D36946 I'm not convinced yet that timer is a good idea there at all.

Oh! Could you explain why you feel this way?

modocache marked an inline comment as done.Aug 23 2017, 8:10 AM

Replace Running member with StartCount.

Looking at https://reviews.llvm.org/D36946 I'm not convinced yet that timer is a good idea there at all.

Oh! Could you explain why you feel this way?

Sorry, I meant D36492 which I commented on.

dblaikie added inline comments.
unittests/Support/TimerTest.cpp
65–75

This seems like an overly general tool given the singular use case - would the test be easier to read if this abstraction weren't used and the operations on the Timer were done directly in the test case? Also possibly testing the state of the Timer after startReentrant has been called but before stopReentrant has been called?

modocache abandoned this revision.Jan 12 2018, 10:32 AM

Thanks for the reviews, everyone! I'll try to come up with something better here, and submit it in its own diff at a later date.