From d7c2e4a77bed665d7ab626d9955c35db8c318e95 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 11 Sep 2019 09:33:07 -0400 Subject: DRTVWR-476: Add Sync class to help with stepwise coroutine tests. Sync is specifically intended for test programs. It is based on an LLScalarCond. The idea is that each of two coroutines can watch for the other to get a chance to run, indicated by incrementing the wrapped int and notifying the wrapped condition_variable. This is less hand-wavy than calling llcoro::suspend() and hoping that the other routine will have had a chance to run. Use Sync in lleventcoro_test.cpp. Also refactor lleventcoro_test.cpp so that instead of a collection of static data requiring a clear() call at start of each individual test function, the relevant data is all part of the test_data struct common to all test functions. Make the helper coroutine functions members of test_data too. Introduce llcoro::logname(), a convenience function to log the name of the currently executing coroutine or "main" if in the thread's main coroutine. --- indra/test/sync.h | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 indra/test/sync.h (limited to 'indra/test/sync.h') diff --git a/indra/test/sync.h b/indra/test/sync.h new file mode 100644 index 0000000000..cafbc034b4 --- /dev/null +++ b/indra/test/sync.h @@ -0,0 +1,85 @@ +/** + * @file sync.h + * @author Nat Goodspeed + * @date 2019-03-13 + * @brief Synchronize coroutines within a test program so we can observe side + * effects. Certain test programs test coroutine synchronization + * mechanisms. Such tests usually want to interleave coroutine + * executions in strictly stepwise fashion. This class supports that + * paradigm. + * + * $LicenseInfo:firstyear=2019&license=viewerlgpl$ + * Copyright (c) 2019, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_SYNC_H) +#define LL_SYNC_H + +#include "llcond.h" +#include "lltut.h" +#include "stringize.h" +#include "llerror.h" +#include "llcoros.h" + +/** + * Instantiate Sync in any test in which we need to suspend one coroutine + * until we're sure that another has had a chance to run. Simply calling + * llcoro::suspend() isn't necessarily enough; that provides a chance for the + * other to run, but doesn't guarantee that it has. If each coroutine is + * consistent about calling Sync::bump() every time it wakes from any + * suspension, Sync::yield() and yield_until() should at least ensure that + * somebody else has had a chance to run. + */ +class Sync +{ + LLScalarCond mCond{0}; + F32Milliseconds mTimeout; + +public: + Sync(F32Milliseconds timeout=F32Milliseconds(10.0f)): + mTimeout(timeout) + {} + + /// Bump mCond by n steps -- ideally, do this every time a participating + /// coroutine wakes up from any suspension. The choice to bump() after + /// resumption rather than just before suspending is worth calling out: + /// this practice relies on the fact that condition_variable::notify_all() + /// merely marks a suspended coroutine ready to run, rather than + /// immediately resuming it. This way, though, even if a coroutine exits + /// before reaching its next suspend point, the other coroutine isn't + /// left waiting forever. + void bump(int n=1) + { + LL_DEBUGS() << llcoro::logname() << " bump(" << n << ") -> " << (mCond.get() + n) << LL_ENDL; + mCond.set_all(mCond.get() + n); + } + + /// suspend until "somebody else" has bumped mCond by n steps + void yield(int n=1) + { + return yield_until(STRINGIZE("Sync::yield_for(" << n << ") timed out after " + << int(mTimeout.value()) << "ms"), + mCond.get() + n); + } + + /// suspend until "somebody else" has bumped mCond to a specific value + void yield_until(int until) + { + return yield_until(STRINGIZE("Sync::yield_until(" << until << ") timed out after " + << int(mTimeout.value()) << "ms"), + until); + } + +private: + void yield_until(const std::string& desc, int until) + { + std::string name(llcoro::logname()); + LL_DEBUGS() << name << " yield_until(" << until << ") suspending" << LL_ENDL; + tut::ensure(name + ' ' + desc, mCond.wait_for_equal(mTimeout, until)); + // each time we wake up, bump mCond + bump(); + } +}; + +#endif /* ! defined(LL_SYNC_H) */ -- cgit v1.2.3 From 3cd2beb97ef0d368d47b0b7efd242b3c709d01af Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 9 Dec 2019 14:33:56 -0500 Subject: DRTVWR-476: Make Sync::bump() atomic, add set() method. Using Sync with multiple threads is trickier than with coroutines. In particular, Sync::bump() was racy (get() and set() as two different operations), and threads were proceeding when they should have waited. Fortunately LLCond, on which Sync is based, already supports atomic update operations. Use that for bump(). But to nail things down even more specifically, add set(n) to complement yield_until(n). Using those methods, there should be no ambiguity about which call in one thread synchronizes with which call in the other thread. --- indra/test/sync.h | 51 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 10 deletions(-) (limited to 'indra/test/sync.h') diff --git a/indra/test/sync.h b/indra/test/sync.h index cafbc034b4..0f0b6cece8 100644 --- a/indra/test/sync.h +++ b/indra/test/sync.h @@ -41,18 +41,49 @@ public: mTimeout(timeout) {} - /// Bump mCond by n steps -- ideally, do this every time a participating - /// coroutine wakes up from any suspension. The choice to bump() after - /// resumption rather than just before suspending is worth calling out: - /// this practice relies on the fact that condition_variable::notify_all() - /// merely marks a suspended coroutine ready to run, rather than - /// immediately resuming it. This way, though, even if a coroutine exits - /// before reaching its next suspend point, the other coroutine isn't - /// left waiting forever. + /** + * Bump mCond by n steps -- ideally, do this every time a participating + * coroutine wakes up from any suspension. The choice to bump() after + * resumption rather than just before suspending is worth calling out: + * this practice relies on the fact that condition_variable::notify_all() + * merely marks a suspended coroutine ready to run, rather than + * immediately resuming it. This way, though, even if a coroutine exits + * before reaching its next suspend point, the other coroutine isn't + * left waiting forever. + */ void bump(int n=1) { - LL_DEBUGS() << llcoro::logname() << " bump(" << n << ") -> " << (mCond.get() + n) << LL_ENDL; - mCond.set_all(mCond.get() + n); + // Calling mCond.set_all(mCond.get() + n) would be great for + // coroutines -- but not so good between kernel threads -- it would be + // racy. Make the increment atomic by calling update_all(), which runs + // the passed lambda within a mutex lock. + int updated; + mCond.update_all( + [&n, &updated](int& data) + { + data += n; + // Capture the new value for possible logging purposes. + updated = data; + }); + // In the multi-threaded case, this log message could be a bit + // misleading, as it will be emitted after waiting threads have + // already awakened. But emitting the log message within the lock + // would seem to hold the lock longer than we really ought. + LL_DEBUGS() << llcoro::logname() << " bump(" << n << ") -> " << updated << LL_ENDL; + } + + /** + * Set mCond to a specific n. Use of bump() and yield() is nicely + * maintainable, since you can insert or delete matching operations in a + * test function and have the rest of the Sync operations continue to + * line up as before. But sometimes you need to get very specific, which + * is where set() and yield_until() come in handy: less maintainable, + * more precise. + */ + void set(int n) + { + LL_DEBUGS() << llcoro::logname() << " set(" << n << ")" << LL_ENDL; + mCond.set_all(n); } /// suspend until "somebody else" has bumped mCond by n steps -- cgit v1.2.3 From d979ba68ee91404d5d3037701e49eb26286109f5 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 3 Apr 2020 10:54:37 -0400 Subject: DRTVWR-476: Use a longer default timeout for Sync class. The timeout is meant to prevent a deadlocked test program from hanging a build. It's not intended to ensure some sort of SLA for the operations under test. Empirically, using a longer timeout helps some test programs. The only downside of increasing the timeout is that if some test does hang, it takes longer to notice. But changes on the order of a few seconds are negligible. --- indra/test/sync.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/test/sync.h') diff --git a/indra/test/sync.h b/indra/test/sync.h index 0f0b6cece8..ca8b7262d6 100644 --- a/indra/test/sync.h +++ b/indra/test/sync.h @@ -37,7 +37,7 @@ class Sync F32Milliseconds mTimeout; public: - Sync(F32Milliseconds timeout=F32Milliseconds(10.0f)): + Sync(F32Milliseconds timeout=F32Milliseconds(10000.0f)): mTimeout(timeout) {} -- cgit v1.2.3