Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: src/plugin/DetachedInitialization.h

Issue 29332660: Issue #3391 - Add detached initialization class
Patch Set: Created Dec. 14, 2015, 6:06 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « adblockplus.gyp ('k') | test/plugin/DetachedInitializationTest.cpp » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: src/plugin/DetachedInitialization.h
===================================================================
new file mode 100644
--- /dev/null
+++ b/src/plugin/DetachedInitialization.h
@@ -0,0 +1,258 @@
+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2015 Eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _DETACHED_INITIALIZATION_H_
+#define _DETACHED_INITIALIZATION_H_
+
+#include <atomic>
+#include <thread>
+#include <mutex>
+#include <chrono>
+#include <condition_variable>
+
+/**
+ * A detached initializer for static class members
sergei 2016/01/29 10:04:21 It would be much better to pull the changes of thi
Eric 2016/01/29 15:33:10 No need.
sergei 2016/02/09 17:38:14 Currently it's very inconvenient to review the cod
+ *
+ * Detached initializers use a separate thread to avoid delays at instantiation.
+ * Variables statically initialized in a DLL cannot, however, contain threads.
+ * Thus such initialization must occur in the constructor at the earliest.
+ *
+ * This kind of initialization acts shares much in common with a singleton.
+ * Rather than constructing a singleton, instead we execute an initialization function.
+ * The multiprocessing and synchronization issues, however, are essentially identical.
+ *
+ * The designated usage of this class:
+ * 1) Define a static class member 'x' of this type with an initializer function.
+ * 2) Call 'x.SpawnInitializer()' in the constructors to run the initializer.
+ * 3) Call 'x.EnsureInitialized()' before relying upon any of the side effects of the initializer.
+ *
+ * The implementation acts much like a multithreaded singleton initialization using a double-checked lock.
+ * See https://en.wikipedia.org/wiki/Double-checked_locking and http://preshing.com/20130922/acquire-and-release-fences/
+ * The flag members of this class enable fast-return paths, that is, a return without obtaining a mutex lock.
+ *
+ * \tparam F
+ * The initializer function
+ *
+ * \par Serialization Guarantees
+ * The visible effect of these function returns are serialized:
+ * - SpawnInitializer() before ThreadMain()
+ * - ThreadMain() before EnsureInitialized()
+ * - If ThreadMain() started, ThreadMain() before destructor
+ */
+template<void (&F)()>
+class DetachedInitializer
+{
+ // "protected" declaration allows white-box subclass for testing
+protected:
+ /**
+ * Set to true after initializer thread successfully constructed; remains true thereafter.
+ */
+ std::atomic<bool> hasInitializerStarted;
+
+ /**
+ * Set to true after initializer function completes; remains true thereafter.
+ */
+ std::atomic<bool> isInitializationComplete;
+
+ /**
+ * Synchronizes order of setting flags and completion of member functions
+ *
+ * Protects separate changes to state flags
+ * - In SpawnInitializer(), only one initializer runs
+ * - In ThreadMain() and EnsureInitialized(), initializer completes and waiters notified
+ * Protects mutual changes to state flags
+ * - Serialize setting flags true: hasInitializerStarted always before isInitializationComplete
+ */
+ std::mutex mutex;
+
+ /**
+ * Synchronizes end of initialization with waiting for intialization to finish.
+ */
+ std::condition_variable cv;
+
+ /**
+ * If not null, an exception thrown by the initializer function
+ * Thrown in EnsureInitialized() if present.
+ */
+ std::exception_ptr initializerException;
+
+ typedef std::unique_lock<std::mutex> UniqueLockType;
sergei 2016/01/29 10:04:22 I think we don't need "Type" suffix.
Eric 2016/01/29 15:33:10 It's the C++ library convention, modified for our
sergei 2016/02/09 17:38:15 Acknowledged.
+ typedef std::lock_guard<std::mutex> LockGuardType;
sergei 2016/01/29 10:04:22 UniqueLockType is used only once, LockGuardType is
Eric 2016/01/29 15:33:11 They're present because the code at the point of u
sergei 2016/02/09 17:38:14 That's very subjective, OK, let's leave them.
+
+ inline bool AlreadyInitialized() // noexcept
sergei 2016/01/29 10:04:22 It seems this method should be `const`.
Eric 2016/01/29 15:33:10 Done.
+ {
+ // Memory fence ensures load starts after any other load or store
+ return isInitializationComplete.load(std::memory_order_acquire);
sergei 2016/01/29 10:04:22 Do we really need to be so specific and not use a
Eric 2016/01/29 15:33:11 Memory fences are absolutely critical for correct
sergei 2016/02/09 17:38:14 That's clear that they are critical, why do we nee
+ }
+
+ inline bool AlreadyStartedInitializer() // noexcept
sergei 2016/01/29 10:04:20 It seems this method should be `const`.
Eric 2016/01/29 15:33:11 Done.
+ {
+ return hasInitializerStarted.load(std::memory_order_acquire);
+ }
+
+ /**
+ * Main function for thread
+ */
+ void ThreadMain()
+ {
+ try
+ {
+ F();
+ }
+ catch (std::exception& e)
sergei 2016/01/29 10:04:21 Compiler generates warning `e` is not used.
sergei 2016/01/29 10:04:23 Why is it not const reference?
Eric 2016/01/29 15:33:10 Done. It's fixed in the subsequent patch set, inc
sergei 2016/02/09 17:38:14 I see that in https://codereview.adblockplus.org/2
+ {
+ // Assert initializer threw an exception
sergei 2016/01/29 10:04:22 This comment is not needed
sergei 2016/02/09 17:38:15 ignored?
+ initializerException = std::current_exception();
+ }
+ /*
+ * Synchronize with reading the completion status in EnsureInitialized().
+ * Serialize return from SpawnInitializer() and this function.
+ */
+ LockGuardType lg(mutex);
sergei 2016/01/29 10:04:20 it would be easier to read if name it `lock` not `
Eric 2016/01/29 15:33:11 Its name doesn't matter, since it's a sentry objec
+ isInitializationComplete.store(true, std::memory_order_release);
+ cv.notify_all();
+ }
+
+ /**
+ * "noexcept" version of EnsureInitialized() used to implement both it and the destructor
+ */
+ void EnsureInitializedNoexcept() // noexcept
+ {
+ try
+ {
+ if (AlreadyInitialized())
+ {
+ return; // fast-return path
+ }
+ UniqueLockType ul(mutex); // won't throw because this class uses only scoped locks
sergei 2016/01/29 10:04:23 it would be easier to read if name it `lock` not `
+ /*
+ * We must always check initialization status under lock.
+ * The loop protects against spurious wake-up calls.
+ */
+ while (!AlreadyInitialized())
+ {
+ cv.wait(ul); // won't throw because mutex is locked
+ }
+ }
+ catch (...)
+ {
+ // Should not reach
sergei 2016/01/29 10:04:21 It would be better to put `assert(false && "Should
Eric 2016/01/29 15:33:11 That would be unnecessary code bloat. There's a p
sergei 2016/02/09 17:38:15 It's not more than the comment, however it helps t
+ }
+ }
+
+public:
+ DetachedInitializer()
+ : hasInitializerStarted(false), isInitializationComplete(false)
+ {}
+
+ /**
+ * \par Precondition
+ * - No external aliases to this instance exist except for the one in the initializer thread
+ *
+ * \par Postcondition
+ * - Ensures that initializer thread is not running before return.
+ */
+ ~DetachedInitializer()
+ {
+ /*
+ * We don't need to acquire a lock here.
+ * By precondition at most the initializer thread is running, and it doesn't touch this flag.
sergei 2016/01/29 10:04:23 We don't need this comment.
Eric 2016/01/29 15:33:10 We do.
+ */
+ if (!AlreadyStartedInitializer())
+ {
+ return;
+ }
+ // Wait for initializer to complete
+ EnsureInitialized();
+ }
sergei 2016/01/29 10:04:21 We should wait in the destructor for the finishing
Eric 2016/01/29 15:33:10 Sergei, we _are_ waiting for the destructor to fin
sergei 2016/02/09 17:38:14 Right, overlooked. However, I would to still wait
+
+ /**
+ * Start an initialization thread if not already initialized and no initialization thread exists.
+ *
+ * \par Postcondition
+ * - if returns, AlreadyStartedInitializer is true
+ * - if throws, AlreadyStartedInitializer is false
sergei 2016/01/29 10:04:21 Why is it false and what happens in EnsureInitiali
Eric 2016/01/29 15:33:11 Trace the code. The only times this function can t
sergei 2016/02/09 17:38:13 Acknowledged.
+ */
+ void SpawnInitializer()
+ {
+ if (AlreadyInitialized() || AlreadyStartedInitializer())
+ {
+ return; // fast-return path
+ }
+ // Assert we have neither completed nor started an initialization thread
+ /*
+ * Try to obtain a lock, giving us permission to start the initialization thread.
+ * try_lock() is not reliable, that is, it can sometimes fail to lock when the mutex is unlocked.
+ * It is, however, guaranteed to succeed eventually.
+ */
+ while (true)
+ {
+ if (mutex.try_lock())
+ {
+ // Assert mutex is locked
+ LockGuardType lg(mutex, std::adopt_lock);
sergei 2016/01/29 10:04:22 Why not just use `std::lock_guard<std::mutex> lock
Eric 2016/01/29 15:33:10 Because that would be incorrect code. There's a r
sergei 2016/02/09 17:38:15 What is the race condition here? If there is a rea
+ /*
+ * Refresh our knowledge of whether the initializer has started, this time under lock.
+ */
+ if (AlreadyStartedInitializer())
+ {
+ return;
+ }
+ /*
+ * This thread constructor is the only statement that can throw in this function.
+ * If it does throw, the initialization-started flag won't be set.
+ * An exception here is thus a soft error, since a future invocation might succeed.
+ */
+ std::thread th([this]() -> void { ThreadMain(); });
sergei 2016/01/29 10:04:20 Can it be written down as `[this]{ ThreadMain(); }
+ // Memory fence ensures store ends before any other load or store
+ hasInitializerStarted.store(true, std::memory_order_release);
+ th.detach(); // Won't throw because the thread is both valid and joinable
sergei 2016/01/29 10:04:22 We should not do that, see comment for the destruc
Eric 2016/01/29 15:33:10 Calling 'detach()' here is the whole point of deta
sergei 2016/02/09 17:38:13 If the thread is a member of the class then initia
+ return;
+ }
+ else
+ {
+ // Assert mutex is not locked
+ if (AlreadyStartedInitializer())
+ {
+ return; // Some other thread might have started the thread in the interim
+ }
+ // One microsecond is usually enough for a mutex to fully reset
+ std::this_thread::sleep_for(std::chrono::microseconds(1));
sergei 2016/01/29 10:04:23 std::this_thread::yield() would be better. FYI: ac
Eric 2016/01/29 15:33:11 Calling 'sleep()' is more conservative. This stat
sergei 2016/02/09 17:38:12 std::this_thread::yield() is more descriptive, so
+ }
+ }
+ }
+
+ /**
+ * Ensure that initialization has completed, blocking if necessary until it is.
+ *
+ * SpawnInitializer() must have returned prior to calling this function.
+ * If not, the function will block indefinitely.
+ *
+ * \throw exception
+ * if the initializer threw an exception, it's rethrown here
+ */
+ void EnsureInitialized()
sergei 2016/01/29 10:04:21 If you require to call firstly SpawnInitializer()
sergei 2016/01/29 10:04:21 It would be better to name it WaitForInitCompletio
Eric 2016/01/29 15:33:11 As I've said so many times before, this is a not a
Eric 2016/01/29 15:33:11 Renaming suggestion rejected. The current name ref
sergei 2016/02/09 17:38:12 It does not ensure that something is initialized i
sergei 2016/02/09 17:38:13 If it's used merely two times then it might be not
+ {
+ EnsureInitializedNoexcept();
+ if (initializerException)
+ {
+ std::rethrow_exception(initializerException);
+ }
+ }
+};
+
+#endif // _DETACHED_INITIALIZATION_H_
« no previous file with comments | « adblockplus.gyp ('k') | test/plugin/DetachedInitializationTest.cpp » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld