This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Add a Timer class to assist performance measurement
ClosedPublic

Authored by Chia-hungDuan on Feb 8 2023, 9:01 PM.

Details

Summary

Add Timer and TimingManager which provide convenient way to meause the
execution time of code snippets. The output looks like,

-- Average Operation Time -- -- Name (# of Calls) --
          1747.2(ns)            popBatch (59)
            92.3(ns)            popBatchImpl (73)
           101.6(ns)              EmptyBatchProcess (5)
          2587.0(ns)            pushBlocksImpl (13)

Note that EmptyBatchProcess is nested under the timer popBatchImpl.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Feb 8 2023, 9:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2023, 9:01 PM
Chia-hungDuan requested review of this revision.Feb 8 2023, 9:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2023, 9:01 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
Chia-hungDuan edited the summary of this revision. (Show Details)Feb 9 2023, 9:23 AM

Add more comments and make the printing interval configurable

Slightly adjust format

Chia-hungDuan edited the summary of this revision. (Show Details)Feb 10 2023, 2:51 PM

Verify the output only on linux

Chia-hungDuan added inline comments.Feb 13 2023, 5:43 PM
compiler-rt/lib/scudo/standalone/timing.h
63

Consider accessing the TimingManager from the parent timer.

  1. Switch to use ScopedString
  2. Change the default size to 50
compiler-rt/lib/scudo/standalone/timing.h
63

Given that Timer can be run without binding to TimingManager, we don't have a simple way to assert the TimingManager is not nullptr in ctor of ScopedTimer. Leave it as it is now

Add ScopedTimer::ignore() to skip some scoped events

cferris requested changes to this revision.Mar 21 2023, 7:38 PM
cferris added inline comments.
compiler-rt/lib/scudo/standalone/timing.h
93

Should this be a CHECK not DCHECK? I would not expect this code to ever really be running in debug mode.

99

Same as above, this seems like it would be better as a CHECK.

110

I think this is supposed to T1.

173

maintenance

201

Don't you need to +1 this value to account for the '\0' terminator?

This revision now requires changes to proceed.Mar 21 2023, 7:38 PM
Chia-hungDuan marked 5 inline comments as done.

Address review comment and move the ignore() from ScopedTimer to Timer

compiler-rt/lib/scudo/standalone/timing.h
93

Yep, you're right! Turned all of them to CHECK

201

Oops, you're right! Done.

cferris accepted this revision.Mar 22 2023, 3:24 PM

LGTM

This revision is now accepted and ready to land.Mar 22 2023, 3:24 PM
This revision was landed with ongoing or failed builds.Mar 23 2023, 12:40 PM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Mar 23 2023, 2:29 PM

Hi @Chia-hungDuan, your change is causing two test failures on a build bot:

https://lab.llvm.org/buildbot/#/builders/247/builds/2812

/usr/bin/ld: /usr/bin/ld: DWARF error: can't find .debug_ranges section.
/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/libclang_rt.scudo_standalone.a(timing.cpp.o): in function `scudo::ScopedTimer::~ScopedTimer()':
timing.cpp:(.text._ZN5scudo11ScopedTimerD0Ev[_ZN5scudo11ScopedTimerD5Ev]+0xc9): undefined reference to `operator delete(void*, unsigned long)'
/usr/bin/ld: /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/libclang_rt.scudo_standalone.a(timing.cpp.o): in function `scudo::Timer::~Timer()':
timing.cpp:(.text._ZN5scudo5TimerD0Ev+0xa0): undefined reference to `operator delete(void*, unsigned long)'
/usr/bin/ld: /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/libclang_rt.scudo_standalone.a(timing.cpp.o):(.data.rel.ro._ZTIN5scudo5TimerE[_ZTIN5scudo5TimerE]+0x0): undefined reference to `vtable for __cxxabiv1::__class_type_info'
/usr/bin/ld: /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/libclang_rt.scudo_standalone.a(timing.cpp.o):(.data.rel.ro._ZTIN5scudo11ScopedTimerE[_ZTIN5scudo11ScopedTimerE]+0x0): undefined reference to `vtable for __cxxabiv1::__si_class_type_info'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Can you take a look?

Hi @Chia-hungDuan, your change is causing two test failures on a build bot:

https://lab.llvm.org/buildbot/#/builders/247/builds/2812

/usr/bin/ld: /usr/bin/ld: DWARF error: can't find .debug_ranges section.
/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/libclang_rt.scudo_standalone.a(timing.cpp.o): in function `scudo::ScopedTimer::~ScopedTimer()':
timing.cpp:(.text._ZN5scudo11ScopedTimerD0Ev[_ZN5scudo11ScopedTimerD5Ev]+0xc9): undefined reference to `operator delete(void*, unsigned long)'
/usr/bin/ld: /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/libclang_rt.scudo_standalone.a(timing.cpp.o): in function `scudo::Timer::~Timer()':
timing.cpp:(.text._ZN5scudo5TimerD0Ev+0xa0): undefined reference to `operator delete(void*, unsigned long)'
/usr/bin/ld: /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/libclang_rt.scudo_standalone.a(timing.cpp.o):(.data.rel.ro._ZTIN5scudo5TimerE[_ZTIN5scudo5TimerE]+0x0): undefined reference to `vtable for __cxxabiv1::__class_type_info'
/usr/bin/ld: /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./lib/clang/17/lib/x86_64-unknown-linux-gnu/libclang_rt.scudo_standalone.a(timing.cpp.o):(.data.rel.ro._ZTIN5scudo11ScopedTimerE[_ZTIN5scudo11ScopedTimerE]+0x0): undefined reference to `vtable for __cxxabiv1::__si_class_type_info'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Can you take a look?

Sorry for that. I'm looking into it

I'm going to revert it first