Page MenuHomePhabricator

[Support] Allow nesting paired calls to {start,stop}Timer
AbandonedPublic

Authored by vsk on May 27 2016, 9:48 AM.

Details

Reviewers
davide
rafael
Summary

The old Timer implementation (before r256258) allowed nesting calls to {start,stop}Timer. Bring that behavior back and add a regression test for it.

Diff Detail

Event Timeline

vsk updated this revision to Diff 58800.May 27 2016, 9:48 AM
vsk retitled this revision from to [Support] Allow nesting paired calls to {start,stop}Timer.
vsk updated this object.
vsk added reviewers: davide, rafael.
vsk added a subscriber: llvm-commits.
davide added inline comments.May 27 2016, 9:59 AM
include/llvm/Support/Timer.h
122

Any reason why you can't query an active timer?

lib/Support/Timer.cpp
155

hmm, imagine the following sequence:
startTimer()
startTimer()
clear()
stop()
stop()

You hit an assertion, no?

rafael edited edge metadata.May 27 2016, 9:59 AM
rafael added a subscriber: rafael.

How hard would it be to change the callers that currently depend on this?

davide edited edge metadata.May 27 2016, 10:05 AM

How hard would it be to change the callers that currently depend on this?

I have a patch that changes the clang timer where I hit this issue. I'm going to put it up for review.
The real worry on my side is that I'm not aware of all the possible cases where we rely on this behavior.
We can just drop Vedant's patch on the floor, and wait until we hit the assertions, and fix the bugs as they show up.
What's your opinion?

It seems pretty odd to have a timer that can be started multiple times. If
we can avoid it I think we should.

Cheers,
Rafael

vsk added a comment.May 27 2016, 10:11 AM

@rafael we don't know how hard it would be to change users which expect to be able to nest {start,stop}Timer. In CodeGenAction.cpp alone it seems like there could be many such users.

The real worry on my side is that I'm not aware of all the possible cases where we rely on this behavior.

Right, that's what I'm concerned about too. I don't see any advantages of keeping the stricter (i.e non-nestable) {start,stop}Timer implementation.

include/llvm/Support/Timer.h
122

You'd get back a negative time, since startTimer() does Time -= CurrentTime.

lib/Support/Timer.cpp
155

Yes. It seems correct to me that you'd hit an assertion in that case. clear() would reset the total time, so there is nothing to stop.

I am OK with this if it is cumbersome to use a non ref counted timer in the
current users.

vsk added a comment.May 27 2016, 10:22 AM

It seems pretty odd to have a timer that can be started multiple times. If
we can avoid it I think we should.

You're right that this is odd behavior, and ISTM the original API avoided advertising it on purpose.

Even so, nesting timers does make sense in the context of recursive functions. If we have e.g: foo() { T.start(); if (...) { foo(); } T.stop(); }, then it's natural for the timer to measure all the time spent in foo, including time spent setting up a timer. Without a change like this, we'd make that more difficult.

I am OK with this if it is cumbersome to use a non ref counted timer in the
current users.

http://reviews.llvm.org/D20735

I think you can close this, Vedant. My fix for clang landed instead.

vsk abandoned this revision.Jul 25 2016, 3:48 PM

@davide thanks, closing