Page MenuHomePhabricator

[Support] Make llvm::Timer reusable
ClosedPublic

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

Details

Summary

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.
include/llvm/Support/Timer.h
65–66

Please drop the duplicated name.

107–112

Don't repeat the name in the comment.

108

Is this reliable? Will it return true in

Foo.startTimer();
Foo.hasTriggered();

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.
include/llvm/Support/Timer.h
108

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
include/llvm/Support/Timer.h
83

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

lib/Support/Timer.cpp
99

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

142

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

147

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 --

lib/Support/Timer.cpp
142

This still prevents us from doing:

T.startTimer()
T.stopTimer()
T.stopTimer()

So I don't think it's dead.

147

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
lib/Support/Timer.cpp
146

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

152

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.

LGTM

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

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

unittests/Support/TimerTest.cpp
13

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
unittests/Support/TimerTest.cpp
13

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:

FAILED: /usr/bin/g++ -DGTEST_HAS_PTHREAD=0 -DGTEST_HAS_RTTI=0 -D_DEBUG
-D_GNU_SOURCE -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS
-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
-I/usr/local/google/home/thakis/src/chrome/src/third_party/llvm/unittests/Support
-Iinclude
-I/usr/local/google/home/thakis/src/chrome/src/third_party/llvm/include
-I/usr/local/google/home/thakis/src/chrome/src/third_party/llvm/utils/unittest/googletest/include

-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
/usr/local/google/home/thakis/src/chrome/src/third_party/llvm/unittests/Support/TimerTest.cpp

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

std::this_thread::sleep_for(std::chrono::milliseconds(1));

...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.