Reverting a merge is sticky: it tells git you never want to see that branch
again. Merging the DRTVWR-546 branch, which contained the revert, into the
glthread branch undid much of the development work on that branch. To restore
it we must revert the revert.
This reverts commit 029b41c041.
That is, when LLViewerFetchedTexture::scheduleCreateTexture() wants to call
createTexture() on the LLImageGLThread, but postCreateTexture() on the main
thread, use the "mainloop" WorkQueue to set up the handshake.
Give ThreadPool a public virtual run() method so a subclass can override with
desired behavior. This necessitates a virtual destructor. Add accessors for
embedded WorkQueue (for post calls), ThreadPool name and width (in threads).
Allow LLSimpleton::createInstance() to forward arguments to the subject
constructor.
Make LLImageGLThread an LLSimpleton - that abstraction didn't yet exist at the
time LLImageGLThread was coded. Also derive from ThreadPool rather than
LLThread. Make it a single-thread "pool" with a very large queue capacity.
Introduce 'string_params' typedef for std::initialization_list<std::string>,
and make logwarns(), loginfos(), logdebugs() and logerrs() accept const
string_params&.
Eliminate the central log() function in llsingleton.cpp that used LL_VLOGS().
To cache the result of a (moderately expensive) Log::shouldLog() call,
LL_VLOGS() wants its CallSite object to be static -- but of course the
shouldLog() result will differ for different ELevel values, so LL_VLOGS()
instantiates a static array of CallSite instances. It seems silly to funnel
distinct logwarns(), etc., functions through a common log() function only to
have LL_VLOGS() tease apart ELevel values again. Instead, make logwarns()
directly invoke LL_WARNS(), and similarly for the rest.
To reduce boilerplate in these distinct functions, teach std::ostream how to
stream a string_params instance by looping over its elements. Then each
logwarns(), etc., function can simply stream its string_params argument to
LL_WARNS() or whichever.
In particular, eliminate the LLERROR_CRASH macro in logerrs(). The fact that
it invokes LL_ERRS() ensures that its LL_ENDL macro will crash the viewer.
Instead of accepting a fixed list of (const char* p1="", etc.), accept
(std::initializer_list<std::string_view>). Accepting a
std::initializer_list<T> in your parameter list allows coding (e.g.)
func({T0, T1, T2, ... });
-- in other words, you can pass the initializer_list a brace-enclosed list of
an arbitrary number of instances of T.
Using std::string_view instead of const char* means we can pass *either* const
char* or std::string. string_view is cheaply constructed from either, allowing
uniform treatment within the function.
Constructing string_view from std::string simply extracts the pointer and
length from the std::string.
Constructing string_view from const char* (e.g. a "string literal") requires
scanning the string for its terminating nul byte -- but that would be
necessary even if the scan were deferred until the function body. Since
string_view stores the length, the scan still happens only once.
Remove call from LLAppViewer::cleanup().
Instead, make each LLSingleton<T>::deleteSingleton() call cleanupSingleton()
just before destroying the instance. Since deleteSingleton() is not a
destructor, it's fine to call cleanupSingleton() from there; and since
deleteAll() calls deleteSingleton() on every remaining instance, the former
cleanupAll() functionality has been subsumed into deleteAll().
Since cleanupSingleton() is now called at exactly one point in the instance's
lifetime, we no longer need a bool indicating whether it has been called.
The previous protocol of calling cleanupAll() before deleteAll() implemented a
two-phase cleanup strategy for the application. That is no longer needed.
Moreover, the cleanupAll() / deleteAll() sequence created a time window during
which individual LLSingleton<T> instances weren't usable (to the extent that
their cleanupSingleton() methods released essential resources) but still
existed -- so a getInstance() call would return the crippled instance rather
than recreating it.
Remove cleanupAll() calls from tests; adjust to new order of expected side
effects: instead of A::cleanupSingleton(), B::cleanupSingleton(), ~A(), ~B(),
now we get A::cleanupSingleton(), ~A(), B::cleanupSingleton(), ~B().
instead of deleteSingleton().
Specifically, clear static SingletonData and remove the instance from the
MasterList in the destructor.
Empirically, some consumers are manually deleting LLSingleton instances,
instead of calling deleteSingleton(). If deleteSingleton() handles cleanup
rather than the destructor, we're left with dangling pointers in the Master
List.
We don't also call cleanupSingleton() from the destructor because only
deleteSingleton() promises to call cleanupSingleton(). Hopefully whoever is
directly deleting an LLSingleton subclass instance isn't relying on
cleanupSingleton().
When calling LLParamSingleton::initParamSingleton() on a secondary thread, use
LLMainThreadTask::dispatch() to construct the instance on the main thread --
as with LLSingleton::getInstance().
Given the viewer's mutually-dependent LLSingletons, given that different
threads might simultaneously request different LLSingletons from such a chain
of circular dependencies, the key to avoiding deadlock is to serialize all
LLSingleton construction on one thread: the main thread. Add comments to
LLSingleton::getInstance() explaining the problem and the solution.
Recast LLSingleton's static SingletonData to use LockStatic. Instead of using
Locker, and simply trusting that every reference to sData is within the
dynamic scope of a Locker instance, LockStatic enforces that: you can only
access SingletonData members via LockStatic.
Reorganize the switch in getInstance() to group the CONSTRUCTING error, the
INITIALIZING/INITIALIZED success case, and the DELETED/UNINITIALIZED
construction case.
When [re]constructing an instance, on the main thread, retain the lock and
call constructSingleton() (and capture_dependency()) directly.
On a secondary thread, unlock LockStatic and use LLMainThreadTask::dispatch()
to call getInstance() on the main thread. Since we might end up enqueuing
multiple such tasks, it's important to let getInstance() notice when the
instance has already been constructed and simply return the existing pointer.
Add loginfos() method, sibling to logerrs(), logwarns() and logdebugs().
Produce loginfos() messages when dispatching to the main thread, when actually
running on the main thread and when resuming the suspended requesting thread.
Make deleteSingleton() manage all associated state, instead of delegating some
of that work to ~LLSingleton(). Now, within LockStatic, extract the instance
pointer and set state to DELETED; that lets subsequent code, which retains the
only remaining pointer to the instance, remove the master-list entry, call the
subclass cleanupSingleton() and destructor without needing to hold the lock.
In fact, entirely remove ~LLSingleton().
Import LLSingletonBase::cleanup_() method to wrap the call to subclass
cleanupSingleton() in try/catch.
Remove cleanupAll() calls from llsingleton_test.cpp, and reorder the success
cases to reflect the fact that T::cleanupSingleton() is called immediately
before ~T() for each distinct LLSingleton subclass T.
When getInstance() on a secondary thread dispatches to the main thread, it
necessarily unlocks its LockStatic lock. But an LLSingleton dependency chain
strongly depends on the function stack on which getInstance() is invoked --
the task dispatched to the main thread doesn't know the dependencies tracked
on the requesting thread stack. So, once the main thread delivers the instance
pointer, the requesting thread captures its own dependencies for that
instance.
Back in the requesting thread, obtaining the current EInitState to pass to
capture_dependencies() would have required relocking LockStatic. Instead, I've
convinced myself that (a) capture_dependencies() only wanted to know
EInitState to produce an error for CONSTRUCTING, and (b) in CONSTRUCTING
state, we never get as far as capture_dependencies() because getInstance()
produces an error first.
Eliminate the EInitState parameter from all capture_dependencies() methods.
Remove the LLSingletonBase::capture_dependency() stanza that tested
EInitState. Make the capture_dependencies() variants that accepted LockStatic
instead accept LLSingletonBase*. That lets getInstance(), in the
LLMainThreadTask case, pass the newly-returned instance pointer.
For symmetry, make pop_initializing() accept LLSingletonBase* as well, instead
of accepting LockStatic and extracting mInstance.
The CONSTRUCTED state was only briefly set between constructSingleton() and
finishInitializing(). But as no consumer code is executed between setting
CONSTRUCTED and setting INITIALIZING, it was impossible to reach the switch
statement in either getInstance() method in state CONSTRUCTED. So there was no
point in state CONSTRUCTED. Remove it.
With CONSTRUCTED gone, we only ever call finishInitializing() right after
constructSingleton(). Merge finishInitializing() into constructSingleton().
Remove warnings about LLSingleton not being thread-safe because, at this point,
we have devoted considerable effort to trying to make it thread-safe.
Add LLSingleton<T>::Locker, a nested class which both provides a function-
static mutex and a scoped lock that uses it. Instantiating Locker, which has a
nullary constructor, replaces the somewhat cumbersome idiom of declaring a
std::unique_lock<std::recursive_mutex> lk(getMutex);
This eliminates (or rather, absorbs) the typedefs and getMutex() method from
LLParamSingleton. Replace explicit std::unique_lock declarations in
LLParamSingleton methods with Locker declarations.
Remove LLSingleton<T>::SingletonInitializer nested struct. Instead of
getInstance() relying on function-static initialization to protect (only)
constructSingleton() calls, explicitly use a Locker instance to cover its
whole scope, and make the UNINITIALIZED case call constructSingleton().
Rearrange cases so that after constructSingleton(), control falls through to
the CONSTRUCTED case and the finishInitializing() call.
Use a Locker instance in other public-facing methods too: instanceExists(),
wasDeleted(), ~LLSingleton(). Make destructor protected so it can only be called
via deleteSingleton() (but must be accessible to subclasses for overrides).
Remove LLSingletonBase::get_master() and get_initializing(), which permitted
directly manipulating the master list and the initializing stack without any
locking mechanism. Replace with get_initializing_size().
Similarly, replace LLSingleton_manage_master::get_initializing() with
get_initializing_size(). Use in constructSingleton() in place of
get_initializing().size().
Remove LLSingletonBase::capture_dependency()'s list_t parameter, which
accepted the list returned by get_initializing(). Encapsulate that retrieval
within the scope of the new lock in capture_dependency().
Add LLSingleton_manage_master::capture_dependency(LLSingletonBase*, EInitState)
to forward (or not) a call to LLSingletonBase::capture_dependency(). Nullary
LLSingleton<T>::capture_dependency() calls new LLSingleton_manage_master method.
Equip LLSingletonBase::MasterList with a mutex of its own, separate from the
one donated by the LLSingleton machinery, to serialize use of MasterList data
members. Introduce MasterList::Lock nested class to lock the MasterList mutex
while providing a reference to the MasterList instance. Introduce subclasses
LockedMaster, which provides a reference to the actual mMaster master list
while holding the MasterList lock; and LockedInitializing, which does the same
for the initializing list. Make mMaster and get_initializing_() private so
that consuming code can *only* access those lists via LockedInitializing and
LockedMaster.
Make MasterList::cleanup_initializing_() private, with a LockedInitializing
public forwarding method. This avoids another call to MasterList::instance(),
and also mandates that the lock is currently held during every call.
Similarly, move LLSingletonBase::log_initializing() to a LockedInitializing
log() method.
(transplanted from dca0f16266c7bddedb51ae7d7dca468ba87060d5)
instead of deleteSingleton().
Specifically, clear static SingletonData and remove the instance from the
MasterList in the destructor.
Empirically, some consumers are manually deleting LLSingleton instances,
instead of calling deleteSingleton(). If deleteSingleton() handles cleanup
rather than the destructor, we're left with dangling pointers in the Master
List.
We don't also call cleanupSingleton() from the destructor because only
deleteSingleton() promises to call cleanupSingleton(). Hopefully whoever is
directly deleting an LLSingleton subclass instance isn't relying on
cleanupSingleton().
When calling LLParamSingleton::initParamSingleton() on a secondary thread, use
LLMainThreadTask::dispatch() to construct the instance on the main thread --
as with LLSingleton::getInstance().
Given the viewer's mutually-dependent LLSingletons, given that different
threads might simultaneously request different LLSingletons from such a chain
of circular dependencies, the key to avoiding deadlock is to serialize all
LLSingleton construction on one thread: the main thread. Add comments to
LLSingleton::getInstance() explaining the problem and the solution.
Recast LLSingleton's static SingletonData to use LockStatic. Instead of using
Locker, and simply trusting that every reference to sData is within the
dynamic scope of a Locker instance, LockStatic enforces that: you can only
access SingletonData members via LockStatic.
Reorganize the switch in getInstance() to group the CONSTRUCTING error, the
INITIALIZING/INITIALIZED success case, and the DELETED/UNINITIALIZED
construction case.
When [re]constructing an instance, on the main thread, retain the lock and
call constructSingleton() (and capture_dependency()) directly.
On a secondary thread, unlock LockStatic and use LLMainThreadTask::dispatch()
to call getInstance() on the main thread. Since we might end up enqueuing
multiple such tasks, it's important to let getInstance() notice when the
instance has already been constructed and simply return the existing pointer.
Add loginfos() method, sibling to logerrs(), logwarns() and logdebugs().
Produce loginfos() messages when dispatching to the main thread, when actually
running on the main thread and when resuming the suspended requesting thread.
Make deleteSingleton() manage all associated state, instead of delegating some
of that work to ~LLSingleton(). Now, within LockStatic, extract the instance
pointer and set state to DELETED; that lets subsequent code, which retains the
only remaining pointer to the instance, remove the master-list entry, call the
subclass cleanupSingleton() and destructor without needing to hold the lock.
In fact, entirely remove ~LLSingleton().
Import LLSingletonBase::cleanup_() method to wrap the call to subclass
cleanupSingleton() in try/catch.
Remove cleanupAll() calls from llsingleton_test.cpp, and reorder the success
cases to reflect the fact that T::cleanupSingleton() is called immediately
before ~T() for each distinct LLSingleton subclass T.
When getInstance() on a secondary thread dispatches to the main thread, it
necessarily unlocks its LockStatic lock. But an LLSingleton dependency chain
strongly depends on the function stack on which getInstance() is invoked --
the task dispatched to the main thread doesn't know the dependencies tracked
on the requesting thread stack. So, once the main thread delivers the instance
pointer, the requesting thread captures its own dependencies for that
instance.
Back in the requesting thread, obtaining the current EInitState to pass to
capture_dependencies() would have required relocking LockStatic. Instead, I've
convinced myself that (a) capture_dependencies() only wanted to know
EInitState to produce an error for CONSTRUCTING, and (b) in CONSTRUCTING
state, we never get as far as capture_dependencies() because getInstance()
produces an error first.
Eliminate the EInitState parameter from all capture_dependencies() methods.
Remove the LLSingletonBase::capture_dependency() stanza that tested
EInitState. Make the capture_dependencies() variants that accepted LockStatic
instead accept LLSingletonBase*. That lets getInstance(), in the
LLMainThreadTask case, pass the newly-returned instance pointer.
For symmetry, make pop_initializing() accept LLSingletonBase* as well, instead
of accepting LockStatic and extracting mInstance.
The CONSTRUCTED state was only briefly set between constructSingleton() and
finishInitializing(). But as no consumer code is executed between setting
CONSTRUCTED and setting INITIALIZING, it was impossible to reach the switch
statement in either getInstance() method in state CONSTRUCTED. So there was no
point in state CONSTRUCTED. Remove it.
With CONSTRUCTED gone, we only ever call finishInitializing() right after
constructSingleton(). Merge finishInitializing() into constructSingleton().
Ansariel