[llvm][NFC] Rework Timer.cpp globals to ensure valid lifetimes #121663
+182
−95
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-clang-driver Author: None (macurtis-amd) ChangesThis is intended to help with flang
With this change, I was able to cherry-pick #107270, uncomment I also noticed that Full diff: https://github.com/llvm/llvm-project/pull/121663.diff 3 Files Affected:
diff --git a/clang/lib/Driver/OffloadBundler.cpp b/clang/lib/Driver/OffloadBundler.cpp
index 87d7303d938c90..2d6bdff0393be5 100644
--- a/clang/lib/Driver/OffloadBundler.cpp
+++ b/clang/lib/Driver/OffloadBundler.cpp
@@ -37,6 +37,7 @@
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MD5.h"
+#include "llvm/Support/ManagedStatic.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
@@ -63,9 +64,17 @@ using namespace llvm;
using namespace llvm::object;
using namespace clang;
-static llvm::TimerGroup
- ClangOffloadBundlerTimerGroup("Clang Offload Bundler Timer Group",
- "Timer group for clang offload bundler");
+namespace {
+struct CreateClangOffloadBundlerTimerGroup {
+ static void *call() {
+ return new TimerGroup("Clang Offload Bundler Timer Group",
+ "Timer group for clang offload bundler");
+ }
+};
+} // namespace
+static llvm::ManagedStatic<llvm::TimerGroup,
+ CreateClangOffloadBundlerTimerGroup>
+ ClangOffloadBundlerTimerGroup;
/// Magic string that marks the existence of offloading data.
#define OFFLOAD_BUNDLER_MAGIC_STR "__CLANG_OFFLOAD_BUNDLE__"
@@ -987,7 +996,7 @@ CompressedOffloadBundle::compress(llvm::compression::Params P,
"Compression not supported");
llvm::Timer HashTimer("Hash Calculation Timer", "Hash calculation time",
- ClangOffloadBundlerTimerGroup);
+ *ClangOffloadBundlerTimerGroup);
if (Verbose)
HashTimer.startTimer();
llvm::MD5 Hash;
@@ -1004,7 +1013,7 @@ CompressedOffloadBundle::compress(llvm::compression::Params P,
Input.getBuffer().size());
llvm::Timer CompressTimer("Compression Timer", "Compression time",
- ClangOffloadBundlerTimerGroup);
+ *ClangOffloadBundlerTimerGroup);
if (Verbose)
CompressTimer.startTimer();
llvm::compression::compress(P, BufferUint8, CompressedBuffer);
@@ -1119,7 +1128,7 @@ CompressedOffloadBundle::decompress(const llvm::MemoryBuffer &Input,
"Unknown compressing method");
llvm::Timer DecompressTimer("Decompression Timer", "Decompression time",
- ClangOffloadBundlerTimerGroup);
+ *ClangOffloadBundlerTimerGroup);
if (Verbose)
DecompressTimer.startTimer();
@@ -1141,7 +1150,7 @@ CompressedOffloadBundle::decompress(const llvm::MemoryBuffer &Input,
// Recalculate MD5 hash for integrity check
llvm::Timer HashRecalcTimer("Hash Recalculation Timer",
"Hash recalculation time",
- ClangOffloadBundlerTimerGroup);
+ *ClangOffloadBundlerTimerGroup);
HashRecalcTimer.startTimer();
llvm::MD5 Hash;
llvm::MD5::MD5Result Result;
diff --git a/llvm/include/llvm/Support/Timer.h b/llvm/include/llvm/Support/Timer.h
index 1a32832b6c6536..c05389332b8045 100644
--- a/llvm/include/llvm/Support/Timer.h
+++ b/llvm/include/llvm/Support/Timer.h
@@ -12,6 +12,7 @@
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/DataTypes.h"
+#include "llvm/Support/Mutex.h"
#include <cassert>
#include <memory>
#include <string>
@@ -19,6 +20,7 @@
namespace llvm {
+class TimerGlobals;
class TimerGroup;
class raw_ostream;
@@ -196,6 +198,10 @@ class TimerGroup {
TimerGroup(const TimerGroup &TG) = delete;
void operator=(const TimerGroup &TG) = delete;
+ friend class TimerGlobals;
+ explicit TimerGroup(StringRef Name, StringRef Description,
+ sys::SmartMutex<true> &lock);
+
public:
explicit TimerGroup(StringRef Name, StringRef Description);
diff --git a/llvm/lib/Support/Timer.cpp b/llvm/lib/Support/Timer.cpp
index 634f27f57b00a2..5c0b5943d31f7d 100644
--- a/llvm/lib/Support/Timer.cpp
+++ b/llvm/lib/Support/Timer.cpp
@@ -38,63 +38,40 @@
using namespace llvm;
-// This ugly hack is brought to you courtesy of constructor/destructor ordering
-// being unspecified by C++. Basically the problem is that a Statistic object
-// gets destroyed, which ends up calling 'GetLibSupportInfoOutputFile()'
-// (below), which calls this function. LibSupportInfoOutputFilename used to be
-// a global variable, but sometimes it would get destroyed before the Statistic,
-// causing havoc to ensue. We "fix" this by creating the string the first time
-// it is needed and never destroying it.
-static ManagedStatic<std::string> LibSupportInfoOutputFilename;
-static std::string &getLibSupportInfoOutputFilename() {
- return *LibSupportInfoOutputFilename;
+//===----------------------------------------------------------------------===//
+// Forward declarations for Managed Timer Globals (mtg) getters.
+//
+// Globals have been placed at the end of the file to restrict direct
+// access. Use of getters also has the benefit of making it a bit more explicit
+// that a global is being used.
+//===----------------------------------------------------------------------===//
+namespace {
+class Name2PairMap;
}
-static ManagedStatic<sys::SmartMutex<true> > TimerLock;
-
-/// Allows llvm::Timer to emit signposts when supported.
-static ManagedStatic<SignpostEmitter> Signposts;
-
-namespace {
-struct CreateTrackSpace {
- static void *call() {
- return new cl::opt<bool>("track-memory",
- cl::desc("Enable -time-passes memory "
- "tracking (this may be slow)"),
- cl::Hidden);
- }
-};
-static ManagedStatic<cl::opt<bool>, CreateTrackSpace> TrackSpace;
-struct CreateInfoOutputFilename {
- static void *call() {
- return new cl::opt<std::string, true>(
- "info-output-file", cl::value_desc("filename"),
- cl::desc("File to append -stats and -timer output to"), cl::Hidden,
- cl::location(getLibSupportInfoOutputFilename()));
- }
-};
-static ManagedStatic<cl::opt<std::string, true>, CreateInfoOutputFilename>
- InfoOutputFilename;
-struct CreateSortTimers {
- static void *call() {
- return new cl::opt<bool>(
- "sort-timers",
- cl::desc("In the report, sort the timers in each group "
- "in wall clock time order"),
- cl::init(true), cl::Hidden);
- }
-};
-ManagedStatic<cl::opt<bool>, CreateSortTimers> SortTimers;
-} // namespace
+namespace mtg {
+static std::string &LibSupportInfoOutputFilename();
+static const std::string &InfoOutputFilename();
+static bool TrackSpace();
+static bool SortTimers();
+static SignpostEmitter &Signposts();
+static sys::SmartMutex<true> &TimerLock();
+static TimerGroup &DefaultTimerGroup();
+static TimerGroup *claimDefaultTimerGroup();
+static Name2PairMap &NamedGroupedTimers();
+} // namespace mtg
+//===----------------------------------------------------------------------===//
+//
+//===----------------------------------------------------------------------===//
void llvm::initTimerOptions() {
- *TrackSpace;
- *InfoOutputFilename;
- *SortTimers;
+ mtg::TrackSpace();
+ mtg::InfoOutputFilename();
+ mtg::SortTimers();
}
std::unique_ptr<raw_ostream> llvm::CreateInfoOutputFile() {
- const std::string &OutputFilename = getLibSupportInfoOutputFilename();
+ const std::string &OutputFilename = mtg::LibSupportInfoOutputFilename();
if (OutputFilename.empty())
return std::make_unique<raw_fd_ostream>(2, false); // stderr.
if (OutputFilename == "-")
@@ -115,22 +92,12 @@ std::unique_ptr<raw_ostream> llvm::CreateInfoOutputFile() {
return std::make_unique<raw_fd_ostream>(2, false); // stderr.
}
-namespace {
-struct CreateDefaultTimerGroup {
- static void *call() {
- return new TimerGroup("misc", "Miscellaneous Ungrouped Timers");
- }
-};
-} // namespace
-static ManagedStatic<TimerGroup, CreateDefaultTimerGroup> DefaultTimerGroup;
-static TimerGroup *getDefaultTimerGroup() { return &*DefaultTimerGroup; }
-
//===----------------------------------------------------------------------===//
// Timer Implementation
//===----------------------------------------------------------------------===//
void Timer::init(StringRef TimerName, StringRef TimerDescription) {
- init(TimerName, TimerDescription, *getDefaultTimerGroup());
+ init(TimerName, TimerDescription, mtg::DefaultTimerGroup());
}
void Timer::init(StringRef TimerName, StringRef TimerDescription,
@@ -149,7 +116,7 @@ Timer::~Timer() {
}
static inline size_t getMemUsage() {
- if (!*TrackSpace)
+ if (!mtg::TrackSpace())
return 0;
return sys::Process::GetMallocUsage();
}
@@ -190,7 +157,7 @@ TimeRecord TimeRecord::getCurrentTime(bool Start) {
void Timer::startTimer() {
assert(!Running && "Cannot start a running timer");
Running = Triggered = true;
- Signposts->startInterval(this, getName());
+ mtg::Signposts().startInterval(this, getName());
StartTime = TimeRecord::getCurrentTime(true);
}
@@ -199,7 +166,7 @@ void Timer::stopTimer() {
Running = false;
Time += TimeRecord::getCurrentTime(false);
Time -= StartTime;
- Signposts->endInterval(this, getName());
+ mtg::Signposts().endInterval(this, getName());
}
void Timer::clear() {
@@ -251,7 +218,7 @@ class Name2PairMap {
Timer &get(StringRef Name, StringRef Description, StringRef GroupName,
StringRef GroupDescription) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
std::pair<TimerGroup*, Name2TimerMap> &GroupEntry = Map[GroupName];
@@ -267,14 +234,13 @@ class Name2PairMap {
}
-static ManagedStatic<Name2PairMap> NamedGroupedTimers;
-
NamedRegionTimer::NamedRegionTimer(StringRef Name, StringRef Description,
StringRef GroupName,
StringRef GroupDescription, bool Enabled)
- : TimeRegion(!Enabled ? nullptr
- : &NamedGroupedTimers->get(Name, Description, GroupName,
- GroupDescription)) {}
+ : TimeRegion(!Enabled ? nullptr
+ : &mtg::NamedGroupedTimers().get(Name, Description,
+ GroupName,
+ GroupDescription)) {}
//===----------------------------------------------------------------------===//
// TimerGroup Implementation
@@ -284,11 +250,12 @@ NamedRegionTimer::NamedRegionTimer(StringRef Name, StringRef Description,
/// ctor/dtor and is protected by the TimerLock lock.
static TimerGroup *TimerGroupList = nullptr;
-TimerGroup::TimerGroup(StringRef Name, StringRef Description)
- : Name(Name.begin(), Name.end()),
- Description(Description.begin(), Description.end()) {
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+ sys::SmartMutex<true> &lock)
+ : Name(Name.begin(), Name.end()),
+ Description(Description.begin(), Description.end()) {
// Add the group to TimerGroupList.
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(lock);
if (TimerGroupList)
TimerGroupList->Prev = &Next;
Next = TimerGroupList;
@@ -296,6 +263,9 @@ TimerGroup::TimerGroup(StringRef Name, StringRef Description)
TimerGroupList = this;
}
+TimerGroup::TimerGroup(StringRef Name, StringRef Description)
+ : TimerGroup(Name, Description, mtg::TimerLock()) {}
+
TimerGroup::TimerGroup(StringRef Name, StringRef Description,
const StringMap<TimeRecord> &Records)
: TimerGroup(Name, Description) {
@@ -313,7 +283,7 @@ TimerGroup::~TimerGroup() {
removeTimer(*FirstTimer);
// Remove the group from the TimerGroupList.
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
*Prev = Next;
if (Next)
Next->Prev = Prev;
@@ -321,7 +291,7 @@ TimerGroup::~TimerGroup() {
void TimerGroup::removeTimer(Timer &T) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
// If the timer was started, move its data to TimersToPrint.
if (T.hasTriggered())
@@ -344,7 +314,7 @@ void TimerGroup::removeTimer(Timer &T) {
}
void TimerGroup::addTimer(Timer &T) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
// Add the timer to our list.
if (FirstTimer)
@@ -356,7 +326,7 @@ void TimerGroup::addTimer(Timer &T) {
void TimerGroup::PrintQueuedTimers(raw_ostream &OS) {
// Perhaps sort the timers in descending order by amount of time taken.
- if (*SortTimers)
+ if (mtg::SortTimers())
llvm::sort(TimersToPrint);
TimeRecord Total;
@@ -374,7 +344,7 @@ void TimerGroup::PrintQueuedTimers(raw_ostream &OS) {
// If this is not an collection of ungrouped times, print the total time.
// Ungrouped timers don't really make sense to add up. We still print the
// TOTAL line to make the percentages make sense.
- if (this != getDefaultTimerGroup())
+ if (this != &mtg::DefaultTimerGroup())
OS << format(" Total Execution Time: %5.4f seconds (%5.4f wall clock)\n",
Total.getProcessTime(), Total.getWallTime());
OS << '\n';
@@ -426,7 +396,7 @@ void TimerGroup::prepareToPrintList(bool ResetTime) {
void TimerGroup::print(raw_ostream &OS, bool ResetAfterPrint) {
{
// After preparing the timers we can free the lock
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
prepareToPrintList(ResetAfterPrint);
}
@@ -436,20 +406,20 @@ void TimerGroup::print(raw_ostream &OS, bool ResetAfterPrint) {
}
void TimerGroup::clear() {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
for (Timer *T = FirstTimer; T; T = T->Next)
T->clear();
}
void TimerGroup::printAll(raw_ostream &OS) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
for (TimerGroup *TG = TimerGroupList; TG; TG = TG->Next)
TG->print(OS);
}
void TimerGroup::clearAll() {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
for (TimerGroup *TG = TimerGroupList; TG; TG = TG->Next)
TG->clear();
}
@@ -466,7 +436,7 @@ void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
}
const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
prepareToPrintList(false);
for (const PrintRecord &R : TimersToPrint) {
@@ -493,17 +463,119 @@ const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
}
const char *TimerGroup::printAllJSONValues(raw_ostream &OS, const char *delim) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
for (TimerGroup *TG = TimerGroupList; TG; TG = TG->Next)
delim = TG->printJSONValues(OS, delim);
return delim;
}
void TimerGroup::constructForStatistics() {
- (void)getLibSupportInfoOutputFilename();
- (void)*NamedGroupedTimers;
+ mtg::LibSupportInfoOutputFilename();
+ mtg::NamedGroupedTimers();
}
std::unique_ptr<TimerGroup> TimerGroup::aquireDefaultGroup() {
- return std::unique_ptr<TimerGroup>(DefaultTimerGroup.claim());
+ return std::unique_ptr<TimerGroup>(mtg::claimDefaultTimerGroup());
+}
+
+//===----------------------------------------------------------------------===//
+// Timer Globals
+//
+// Previously, these were independent ManagedStatics. This led to bugs because
+// there are dependencies between the globals, but no reliable mechanism to
+// control relative lifetimes.
+//
+// Placing the globals within one class instance lets us control the lifetimes
+// of the various data members and ensure that no global uses another that has
+// been deleted.
+//
+// Globals fall into two categories. First are simple data types and
+// command-line options. These are cheap to construct and/or required early
+// during launch. They are created when the ManagedTimerGlobals singleton is
+// constructed. Second are types that are more expensive to construct or not
+// needed until later during compilation. These are lazily constructed in order
+// to reduce launch time.
+//===----------------------------------------------------------------------===//
+class llvm::TimerGlobals {
+public:
+ std::string LibSupportInfoOutputFilename;
+ cl::opt<std::string, true> InfoOutputFilename;
+ cl::opt<bool> TrackSpace;
+ cl::opt<bool> SortTimers;
+
+private:
+ // Order of these members and initialization below is important. For example
+ // the DefaultTimerGroup uses the TimerLock. Most of these also depend on the
+ // options above.
+ std::unique_ptr<SignpostEmitter> SignpostsPtr;
+ std::unique_ptr<sys::SmartMutex<true>> TimerLockPtr;
+ std::unique_ptr<TimerGroup> DefaultTimerGroupPtr;
+ std::unique_ptr<Name2PairMap> NamedGroupedTimersPtr;
+ std::once_flag InitDeferredFlag;
+ TimerGlobals &initDeferred() {
+ std::call_once(InitDeferredFlag, [this]() {
+ SignpostsPtr = std::make_unique<SignpostEmitter>();
+ TimerLockPtr = std::make_unique<sys::SmartMutex<true>>();
+ DefaultTimerGroupPtr.reset(new TimerGroup(
+ "misc", "Miscellaneous Ungrouped Timers", *TimerLockPtr));
+ NamedGroupedTimersPtr = std::make_unique<Name2PairMap>();
+ });
+ return *this;
+ }
+
+public:
+ SignpostEmitter &Signposts() { return *initDeferred().SignpostsPtr; }
+ sys::SmartMutex<true> &TimerLock() { return *initDeferred().TimerLockPtr; }
+ TimerGroup &DefaultTimerGroup() {
+ return *initDeferred().DefaultTimerGroupPtr;
+ }
+ TimerGroup *claimDefaultTimerGroup() {
+ return initDeferred().DefaultTimerGroupPtr.release();
+ }
+ Name2PairMap &NamedGroupedTimers() {
+ return *initDeferred().NamedGroupedTimersPtr;
+ }
+
+public:
+ TimerGlobals()
+ : InfoOutputFilename(
+ "info-output-file", cl::value_desc("filename"),
+ cl::desc("File to append -stats and -timer output to"), cl::Hidden,
+ cl::location(LibSupportInfoOutputFilename)),
+ TrackSpace(
+ "track-memory",
+ cl::desc("Enable -time-passes memory tracking (this may be slow)"),
+ cl::Hidden),
+ SortTimers(
+ "sort-timers",
+ cl::desc(
+ "In the report, sort the timers in each group in wall clock"
+ " time order"),
+ cl::init(true), cl::Hidden) {}
+};
+
+static ManagedStatic<TimerGlobals> ManagedTimerGlobals;
+
+static std::string &mtg::LibSupportInfoOutputFilename() {
+ return ManagedTimerGlobals->LibSupportInfoOutputFilename;
+}
+static const std::string &mtg::InfoOutputFilename() {
+ return ManagedTimerGlobals->InfoOutputFilename.getValue();
+}
+static bool mtg::TrackSpace() { return ManagedTimerGlobals->TrackSpace; };
+static bool mtg::SortTimers() { return ManagedTimerGlobals->SortTimers; }
+static SignpostEmitter &mtg::Signposts() {
+ return ManagedTimerGlobals->Signposts();
+}
+static sys::SmartMutex<true> &mtg::TimerLock() {
+ return ManagedTimerGlobals->TimerLock();
+}
+static TimerGroup &mtg::DefaultTimerGroup() {
+ return ManagedTimerGlobals->DefaultTimerGroup();
+}
+static TimerGroup *mtg::claimDefaultTimerGroup() {
+ return ManagedTimerGlobals->claimDefaultTimerGroup();
+}
+static Name2PairMap &mtg::NamedGroupedTimers() {
+ return ManagedTimerGlobals->NamedGroupedTimers();
}
|
macurtis-amd
requested review from
nhaehnle,
JDevlieghere,
tarunprabhu and
ronlieb
tarunprabhu
reviewed
Jan 6, 2025
| static SignpostEmitter &Signposts(); | ||
| static sys::SmartMutex<true> &TimerLock(); | ||
| static TimerGroup &DefaultTimerGroup(); | ||
| static TimerGroup *claimDefaultTimerGroup(); |
There was a problem hiding this comment.
Is there a reason that this has different casing than the other functions?
Could all of them be made "getters" in camel case since they are functions anyway?
| : Name(Name.begin(), Name.end()), | ||
| Description(Description.begin(), Description.end()) { | ||
| TimerGroup::TimerGroup(StringRef Name, StringRef Description, | ||
| sys::SmartMutex<true> &lock) |
There was a problem hiding this comment.
Should lock be Lock here in order to be consistent with the other parameter names?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is intended to help with flang
-ftime-reportsupport:With this change, I was able to cherry-pick #107270, uncomment
llvm::TimePassesIsEnabled = true;and compile with-ftime-report.I also noticed that
clang/lib/Driver/OffloadBundler.cppwas statically constructing aTimerGroupand changed it to lazily construct via ManagedStatic.