Page MenuHomePhabricator

[Support] Make llvm::Timer reusable

Authored by vsk on Dec 17 2015, 11:07 AM.



Make llvm::Timer reusable by allowing it to be started + stopped repeatedly.

This gets rid of an awkward static variable (ActiveTimers) and makes the API more flexible. It also adds a unittest, which I can extend in a later patch.

Rationale: In my use case, I have a long-running tool which periodically calls into llvm. I need to time a few regions multiple times. It makes life much easier to use a single Timer for this.

This patch also cleans up some documentation -- that will go in a separate NFC commit.

Diff Detail

Event Timeline

vsk updated this revision to Diff 43158.Dec 17 2015, 11:07 AM
vsk retitled this revision from to [Support] Make llvm::Timer reusable.
vsk updated this object.
vsk added reviewers: bruno, mehdi_amini.
vsk added a subscriber: llvm-commits.
vsk updated this revision to Diff 43174.Dec 17 2015, 12:42 PM
  • Make TimerGroup use accessors instead of accessing Timer directly.
  • Retain the old T.Started behavior with T.hasTriggered(). This checks whether the Timer has ever been started. Fixes -ftime-report behavior.
vsk updated this revision to Diff 43274.Dec 18 2015, 3:19 PM
vsk updated this object.
vsk edited reviewers, added: chapuni; removed: mehdi_amini, bruno.
  • Added unittest, made some cosmetic changes.
  • Removed bruno and joker.eph as reviewers since they are on vacation, added chapuni.
vsk updated this revision to Diff 43275.Dec 18 2015, 3:21 PM

I forgot to include the unittest in the previous diff.

vsk updated this revision to Diff 43400.Dec 21 2015, 2:12 PM
vsk removed a reviewer: chapuni.
  • Make clear() reset the timer, instead of just the total time field.
  • Fix up the unit test.
vsk updated this revision to Diff 43401.Dec 21 2015, 2:15 PM
  • Attached the complete diff this time.
rafael added inline comments.

Please drop the duplicated name.


Don't repeat the name in the comment.


Is this reliable? Will it return true in


That is, if hasTriggered is called immediately after the start.

vsk updated this revision to Diff 43404.Dec 21 2015, 2:51 PM
vsk marked 3 inline comments as done.
  • Used a new bool to implement hasTriggered() for simplicity.

Yes it will return true in that scenario. I added a bool to make this clearer, and a CheckIfTriggered unit test.

rafael added inline comments.Dec 21 2015, 3:10 PM

Don't you have to set it to false in init()?


This is a nice independent cleanup. Please commit and rebase.


Deleting this is a nice independent cleanup. Please commit and rebase. Looks like it is dead since 99841.


These two lines are just:

+= (TimeRecord::getCurrentTime(false) - StartTime);

vsk marked 2 inline comments as done.Dec 21 2015, 3:51 PM

Thanks Rafael. Comments inline, updated diff coming --


This still prevents us from doing:


So I don't think it's dead.


There is no operator- for TimeRecord. Should we have one, and if so, is that a separate commit?

vsk updated this revision to Diff 43411.Dec 21 2015, 3:52 PM
  • Addressed Rafael's comments.
  • Rebased to master.
rafael added inline comments.Dec 22 2015, 8:52 AM

"Running" might be a better name for Started now.


The comment says "This does not affect the result of hasTriggered()."

vsk updated this revision to Diff 43450.Dec 22 2015, 9:13 AM
vsk marked 2 inline comments as done.
  • Style fixes per Rafael's comments.
rafael accepted this revision.Dec 22 2015, 9:24 AM
rafael added a reviewer: rafael.


This revision is now accepted and ready to land.Dec 22 2015, 9:24 AM

If you can't fix this quickly, consider reverting.


This doesn't build on Windows:

FAILED: C:\PROGRA~2\MICROS~1.0\VC\bin\cl.exe /nologo /TP /DWIN32 /D_WINDOWS -wd4141 -wd4146 -wd4180 -wd4244 -wd4258 -wd4267 -wd4291 -wd4345 -wd4351 -wd4355 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4800 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4324 -w14062 -we4238 /W4 /Zc:inline /MD /O2 /Ob2 -Iunittests\Support -IC:\buildbot\slave-config\clang-x86-win2008-selfhost\llvm\unittests\Support -Iinclude -IC:\buildbot\slave-config\clang-x86-win2008-selfhost\llvm\include -IC:\buildbot\slave-config\clang-x86-win2008-selfhost\llvm\utils\unittest\googletest\include -UNDEBUG /EHs-c- /GR- /showIncludes -DGTEST_HAS_RTTI=0 -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_DEBUG_POINTER_IMPL="" -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS -DSTDC_LIMIT_MACROS /Founittests\Support\CMakeFiles\SupportTests.dir\TimerTest.cpp.obj /Fdunittests\Support\CMakeFiles\SupportTests.dir\ /FS -c C:\buildbot\slave-config\clang-x86-win2008-selfhost\llvm\unittests\Support\TimerTest.cpp
C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\INCLUDE\concrt.h(313) : warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc
C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\INCLUDE\concrt.h(4774) : error C3861: '
uncaught_exception': identifier not found

See the goop in include/llvm/Support/ThreadPool.h for what you probably need to do here.

vsk added inline comments.Dec 22 2015, 3:21 PM

Sorry for the delay. I got bitten by hfs case-insensitivity in r256290, hopefully r256292 fixes things.

Now it fails to build on Linux for me:

-D__STDC_LIMIT_MACROS -fPIC -fvisibility-inlines-hidden -Wall -W
-Wno-unused-parameter -Wwrite-strings -Wcast-qual
-Wno-missing-field-initializers -pedantic -Wno-long-long
-Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -std=c++11
-ffunction-sections -fdata-sections -O3 -Iunittests/Support

-UNDEBUG  -Wno-variadic-macros -fno-exceptions -fno-rtti -MMD -MT

unittests/Support/CMakeFiles/SupportTests.dir/TimerTest.cpp.o -MF
unittests/Support/CMakeFiles/SupportTests.dir/TimerTest.cpp.o.d -o
unittests/Support/CMakeFiles/SupportTests.dir/TimerTest.cpp.o -c

chrome/src/third_party/llvm/unittests/Support/TimerTest.cpp:29:8: error:
‘std::this_thread’ has not been declared


...because llvm/Support/thread.h only includes <thread> if LLVM_ENABLE_THREADS is defined.

I tried to fix that problem in r256308. Maybe you can take a look at the FIXME in that revision.

vsk closed this revision.Feb 24 2016, 6:27 PM

More phab GC'ing.