From 285613b892f41d0c72c03b8551dd003f61a5f2be Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 3 Jun 2009 21:38:21 +0000 Subject: DEV-32777: Introduce LLCoros, an LLSingleton registry of named coroutine instances. LLCoros::launch() intends to address three issues: - ownership of coroutine instance - cleanup of coroutine instance when it terminates - central place to twiddle MSVC optimizations to bypass DEV-32777 crash. Initially coded on Mac; will address the third bullet on Windows. Adapt listenerNameForCoro() to consult LLCoros::getName() if applicable. Change LLLogin::Impl::connect() to use LLCoros::launch(). LLCoros::getName() relies on patch to boost::coroutines::coroutine::self to introduce get_id(). --- indra/llcommon/llcoros.cpp | 107 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 indra/llcommon/llcoros.cpp (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp new file mode 100644 index 0000000000..6fa6ae8f1a --- /dev/null +++ b/indra/llcommon/llcoros.cpp @@ -0,0 +1,107 @@ +/** + * @file llcoros.cpp + * @author Nat Goodspeed + * @date 2009-06-03 + * @brief Implementation for llcoros. + * + * $LicenseInfo:firstyear=2009&license=viewergpl$ + * Copyright (c) 2009, Linden Research, Inc. + * $/LicenseInfo$ + */ + +// Precompiled header +#include "linden_common.h" +// associated header +#include "llcoros.h" +// STL headers +// std headers +// external library headers +#include +// other Linden headers +#include "llevents.h" +#include "llerror.h" +#include "stringize.h" + +LLCoros::LLCoros() +{ + // Register our cleanup() method for "mainloop" ticks + LLEventPumps::instance().obtain("mainloop").listen( + "LLCoros", boost::bind(&LLCoros::cleanup, this, _1)); +} + +bool LLCoros::cleanup(const LLSD&) +{ + // Walk the mCoros map, checking and removing completed coroutines. + for (CoroMap::iterator mi(mCoros.begin()), mend(mCoros.end()); mi != mend; ) + { + // Has this coroutine exited (normal return, exception, exit() call) + // since last tick? + if (mi->second->exited()) + { + LL_INFOS("LLCoros") << "LLCoros: cleaning up coroutine " << mi->first << LL_ENDL; + // The erase() call will invalidate its passed iterator value -- + // so increment mi FIRST -- but pass its original value to + // erase(). This is what postincrement is all about. + mCoros.erase(mi++); + } + else + { + // Still live, just skip this entry as if incrementing at the top + // of the loop as usual. + ++mi; + } + } + return false; +} + +std::string LLCoros::generateDistinctName(const std::string& prefix) const +{ + // Allowing empty name would make getName()'s not-found return ambiguous. + if (prefix.empty()) + { + LL_ERRS("LLCoros") << "LLCoros::launch(): pass non-empty name string" << LL_ENDL; + } + + // If the specified name isn't already in the map, just use that. + std::string name(prefix); + + // Find the lowest numeric suffix that doesn't collide with an existing + // entry. Start with 2 just to make it more intuitive for any interested + // parties: e.g. "joe", "joe2", "joe3"... + for (int i = 2; ; name = STRINGIZE(prefix << i++)) + { + if (mCoros.find(name) == mCoros.end()) + { + LL_INFOS("LLCoros") << "LLCoros: launching coroutine " << name << LL_ENDL; + return name; + } + } +} + +bool LLCoros::kill(const std::string& name) +{ + CoroMap::iterator found = mCoros.find(name); + if (found == mCoros.end()) + { + return false; + } + // Because this is a boost::ptr_map, erasing the map entry also destroys + // the referenced heap object, in this case an LLCoro. That will destroy + // the contained boost::coroutine object, which will terminate the coroutine. + mCoros.erase(found); + return true; +} + +std::string LLCoros::getNameByID(const void* self_id) const +{ + // Walk the existing coroutines, looking for one from which the 'self_id' + // passed to us comes. + for (CoroMap::const_iterator mi(mCoros.begin()), mend(mCoros.end()); mi != mend; ++mi) + { + if (mi->second->owns_self_id(self_id)) + { + return mi->first; + } + } + return ""; +} -- cgit v1.3 From 820d4a20d1b9c9a3e562b925ba59a6addcffa558 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 4 Jun 2009 16:01:40 +0000 Subject: DEV-32777: Use a canonical boost::coroutines::coroutine signature, relying on boost::bind() to pass any other coroutine arguments. This allows us to remove the LLCoroBase and LLCoro constructs, directly storing a coroutine object in our ptr_map. It also allows us to remove the multiple launch() overloads for multiple arguments. Finally, it lets us move most launch() functionality into a non-template method. --- indra/llcommon/llcoros.cpp | 17 +++- indra/llcommon/llcoros.h | 136 +++++++++--------------------- indra/viewer_components/login/lllogin.cpp | 10 +-- 3 files changed, 58 insertions(+), 105 deletions(-) (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index 6fa6ae8f1a..5d23e1d284 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -54,6 +54,15 @@ bool LLCoros::cleanup(const LLSD&) return false; } +std::string LLCoros::launchImpl(const std::string& prefix, coro* newCoro) +{ + std::string name(generateDistinctName(prefix)); + mCoros.insert(name, newCoro); + /* Run the coroutine until its first wait, then return here */ + (*newCoro)(std::nothrow); + return name; +} + std::string LLCoros::generateDistinctName(const std::string& prefix) const { // Allowing empty name would make getName()'s not-found return ambiguous. @@ -86,8 +95,8 @@ bool LLCoros::kill(const std::string& name) return false; } // Because this is a boost::ptr_map, erasing the map entry also destroys - // the referenced heap object, in this case an LLCoro. That will destroy - // the contained boost::coroutine object, which will terminate the coroutine. + // the referenced heap object, in this case the boost::coroutine object, + // which will terminate the coroutine. mCoros.erase(found); return true; } @@ -98,7 +107,9 @@ std::string LLCoros::getNameByID(const void* self_id) const // passed to us comes. for (CoroMap::const_iterator mi(mCoros.begin()), mend(mCoros.end()); mi != mend; ++mi) { - if (mi->second->owns_self_id(self_id)) + namespace coro_private = boost::coroutines::detail; + if (static_cast(coro_private::coroutine_accessor::get_impl(const_cast(*mi->second)).get()) + == self_id) { return mi->first; } diff --git a/indra/llcommon/llcoros.h b/indra/llcommon/llcoros.h index dfbe76932c..6b07ba4105 100644 --- a/indra/llcommon/llcoros.h +++ b/indra/llcommon/llcoros.h @@ -21,53 +21,16 @@ #include #include -/// Base class for each coroutine -struct LLCoroBase -{ - LLCoroBase() {} - virtual ~LLCoroBase() {} - - virtual bool exited() const = 0; - template - bool owns_self(const COROUTINE_SELF& self) const - { - return owns_self_id(self.get_id()); - } - - virtual bool owns_self_id(const void* self_id) const = 0; -}; - -/// Template subclass to accommodate different boost::coroutine signatures -template -struct LLCoro: public LLCoroBase -{ - template - LLCoro(const CALLABLE& callable): - mCoro(callable) - {} - - virtual bool exited() const { return mCoro.exited(); } - - COROUTINE mCoro; - - virtual bool owns_self_id(const void* self_id) const - { - namespace coro_private = boost::coroutines::detail; - return static_cast(coro_private::coroutine_accessor::get_impl(const_cast(mCoro)).get()) - == self_id; - } -}; - /** * Registry of named Boost.Coroutine instances * - * The Boost.Coroutine library supports the general case of a coroutine accepting - * arbitrary parameters and yielding multiple (sets of) results. For such use - * cases, it's natural for the invoking code to retain the coroutine instance: - * the consumer repeatedly calls back into the coroutine until it yields its - * next result. + * The Boost.Coroutine library supports the general case of a coroutine + * accepting arbitrary parameters and yielding multiple (sets of) results. For + * such use cases, it's natural for the invoking code to retain the coroutine + * instance: the consumer repeatedly calls into the coroutine, perhaps passing + * new parameter values, prompting it to yield its next result. * - * Our typical coroutine usage is a bit different, though. For us, coroutines + * Our typical coroutine usage is different, though. For us, coroutines * provide an alternative to the @c Responder pattern. Our typical coroutine * has @c void return, invoked in fire-and-forget mode: the handler for some * user gesture launches the coroutine and promptly returns to the main loop. @@ -98,7 +61,11 @@ struct LLCoro: public LLCoroBase class LLCoros: public LLSingleton { public: - /*------------------------------ launch() ------------------------------*/ + /// Canonical boost::coroutines::coroutine signature we use + typedef boost::coroutines::coroutine coro; + /// Canonical 'self' type + typedef coro::self self; + /** * Create and start running a new coroutine with specified name. The name * string you pass is a suggestion; it will be tweaked for uniqueness. The @@ -106,68 +73,44 @@ public: * * Usage looks like this, for (e.g.) two coroutine parameters: * @code - * typedef boost::coroutines::coroutine coro_type; - * std::string name = LLCoros::instance().launch( - * "mycoro", boost::bind(&MyClass::method, this, _1, _2, _3), - * "somestring", LLSD(17)); + * class MyClass + * { + * public: + * ... + * // Do NOT NOT NOT accept reference params other than 'self'! + * // Pass by value only! + * void myCoroutineMethod(LLCoros::self& self, std::string, LLSD); + * ... + * }; + * ... + * std::string name = LLCoros::instance().launch( + * "mycoro", boost::bind(&MyClass::myCoroutineMethod, this, _1, + * "somestring", LLSD(17)); * @endcode * - * In other words, you must specify: + * Your function/method must accept LLCoros::self& as its first parameter. + * It can accept any other parameters you want -- but ONLY BY VALUE! + * Other reference parameters are a BAD IDEA! You Have Been Warned. See + * DEV-32777 comments for an explanation. * - * * the desired boost::coroutines::coroutine type, to whose - * signature the initial coro_type::self& parameter is - * implicitly added - * * the suggested name string for the new coroutine instance - * * the callable to be run, e.g. boost::bind() expression for a - * class method -- not forgetting to add _1 for the - * coro_type::self& parameter - * * the actual parameters to be passed to that callable after the - * implicit coro_type::self& parameter + * Pass a callable that accepts the single LLCoros::self& parameter. It + * may work to pass a free function whose only parameter is 'self'; for + * all other cases use boost::bind(). Of course, for a non-static class + * method, the first parameter must be the class instance. Use the + * placeholder _1 for the 'self' parameter. Any other parameters should be + * passed via the bind() expression. * * launch() tweaks the suggested name so it won't collide with any * existing coroutine instance, creates the coroutine instance, registers * it with the tweaked name and runs it until its first wait. At that * point it returns the tweaked name. - * - * Use of a typedef for the coroutine type is recommended, because you - * must restate it for the callable's first parameter. - * - * @note - * launch() only accepts const-reference parameters. Once we can assume - * C++0x features on every platform, we'll have so-called "perfect - * forwarding" and variadic templates and other such ponies, and can - * support an arbitrary number of truly arbitrary parameter types. But for - * now, we'll stick with const reference params. N.B. Passing a non-const - * reference to a local variable into a coroutine seems like a @em really - * bad idea: the local variable will be destroyed during the lifetime of - * the coroutine. */ - // Use the preprocessor to generate launch() overloads accepting 0, 1, - // ..., BOOST_COROUTINE_ARG_MAX const ref params of arbitrary type. -#define BOOST_PP_LOCAL_MACRO(n) \ - template \ - std::string launch(const std::string& prefix, const CALLABLE& callable \ - BOOST_PP_COMMA_IF(n) \ - BOOST_PP_ENUM_BINARY_PARAMS(n, const T, & p)) \ - { \ - std::string name(generateDistinctName(prefix)); \ - LLCoro* ptr = new LLCoro(callable); \ - mCoros.insert(name, ptr); \ - /* Run the coroutine until its first wait, then return here */ \ - ptr->mCoro(std::nothrow \ - BOOST_PP_COMMA_IF(n) \ - BOOST_PP_ENUM_PARAMS(n, p)); \ - return name; \ + template + std::string launch(const std::string& prefix, const CALLABLE& callable) + { + return launchImpl(prefix, new coro(callable)); } -#define BOOST_PP_LOCAL_LIMITS (0, BOOST_COROUTINE_ARG_MAX) -#include BOOST_PP_LOCAL_ITERATE() -#undef BOOST_PP_LOCAL_MACRO -#undef BOOST_PP_LOCAL_LIMITS - /*----------------------- end of launch() family -----------------------*/ - /** * Abort a running coroutine by name. Normally, when a coroutine either * runs to completion or terminates with an exception, LLCoros quietly @@ -195,10 +138,11 @@ public: private: friend class LLSingleton; LLCoros(); + std::string launchImpl(const std::string& prefix, coro* newCoro); std::string generateDistinctName(const std::string& prefix) const; bool cleanup(const LLSD&); - typedef boost::ptr_map CoroMap; + typedef boost::ptr_map CoroMap; CoroMap mCoros; }; diff --git a/indra/viewer_components/login/lllogin.cpp b/indra/viewer_components/login/lllogin.cpp index 92384a1c5b..c0d35f31d3 100644 --- a/indra/viewer_components/login/lllogin.cpp +++ b/indra/viewer_components/login/lllogin.cpp @@ -105,12 +105,10 @@ private: return response; } - typedef boost::coroutines::coroutine coroutine_type; - // In a coroutine's top-level function args, do NOT NOT NOT accept // references (const or otherwise) to anything but the self argument! Pass // by value only! - void login_(coroutine_type::self& self, std::string uri, LLSD credentials); + void login_(LLCoros::self& self, std::string uri, LLSD credentials); LLEventStream mPump; LLSD mAuthResponse, mValidAuthResponse; @@ -121,11 +119,11 @@ void LLLogin::Impl::connect(const std::string& uri, const LLSD& credentials) // Launch a coroutine with our login_() method. Run the coroutine until // its first wait; at that point, return here. std::string coroname = - LLCoros::instance().launch("LLLogin::Impl::login_", - boost::bind(&Impl::login_, this, _1, uri, credentials)); + LLCoros::instance().launch("LLLogin::Impl::login_", + boost::bind(&Impl::login_, this, _1, uri, credentials)); } -void LLLogin::Impl::login_(coroutine_type::self& self, std::string uri, LLSD credentials) +void LLLogin::Impl::login_(LLCoros::self& self, std::string uri, LLSD credentials) { LL_INFOS("LLLogin") << "Entering coroutine " << LLCoros::instance().getName(self) << " with uri '" << uri << "', credentials " << credentials << LL_ENDL; -- cgit v1.3 From ec52e19dd16908acd72b78720880391a74ee8886 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 25 Sep 2009 10:55:25 -0400 Subject: DEV-32777, QAR-1619: Disable MSVC Release-build optimization for LLCoros::launchImpl(). This fixes the Release-build crash in lllogin_test.cpp. --- indra/llcommon/llcoros.cpp | 37 ++++++++++++++++------ .../viewer_components/login/tests/lllogin_test.cpp | 2 +- 2 files changed, 29 insertions(+), 10 deletions(-) (limited to 'indra/llcommon/llcoros.cpp') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index 5d23e1d284..377bfaa247 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -54,15 +54,6 @@ bool LLCoros::cleanup(const LLSD&) return false; } -std::string LLCoros::launchImpl(const std::string& prefix, coro* newCoro) -{ - std::string name(generateDistinctName(prefix)); - mCoros.insert(name, newCoro); - /* Run the coroutine until its first wait, then return here */ - (*newCoro)(std::nothrow); - return name; -} - std::string LLCoros::generateDistinctName(const std::string& prefix) const { // Allowing empty name would make getName()'s not-found return ambiguous. @@ -116,3 +107,31 @@ std::string LLCoros::getNameByID(const void* self_id) const } return ""; } + +/***************************************************************************** +* MUST BE LAST +*****************************************************************************/ +// Turn off MSVC optimizations for just LLCoros::launchImpl() -- see +// DEV-32777. But MSVC doesn't support push/pop for optimization flags as it +// does for warning suppression, and we really don't want to force +// optimization ON for other code even in Debug or RelWithDebInfo builds. + +#if LL_MSVC +// work around broken optimizations +#pragma warning(disable: 4748) +#pragma optimize("", off) +#endif // LL_MSVC + +std::string LLCoros::launchImpl(const std::string& prefix, coro* newCoro) +{ + std::string name(generateDistinctName(prefix)); + mCoros.insert(name, newCoro); + /* Run the coroutine until its first wait, then return here */ + (*newCoro)(std::nothrow); + return name; +} + +#if LL_MSVC +// reenable optimizations +#pragma optimize("", on) +#endif // LL_MSVC diff --git a/indra/viewer_components/login/tests/lllogin_test.cpp b/indra/viewer_components/login/tests/lllogin_test.cpp index e43065d49f..51f00c8344 100644 --- a/indra/viewer_components/login/tests/lllogin_test.cpp +++ b/indra/viewer_components/login/tests/lllogin_test.cpp @@ -56,7 +56,7 @@ public: return pump.listen(mName, boost::bind(&LoginListener::call, this, _1)); } - LLSD lastEvent() { return mLastEvent; } + LLSD lastEvent() const { return mLastEvent; } friend std::ostream& operator<<(std::ostream& out, const LoginListener& listener) { -- cgit v1.3