From 831b87f2342df0ee2d13c466b400245bdf1c04f3 Mon Sep 17 00:00:00 2001 From: kosak Date: Sun, 19 Jul 2015 22:33:19 +0000 Subject: [PATCH] Do not create an extra default instance of T when constructing a ThreadLocal. --- include/gtest/internal/gtest-port.h | 88 +++++++++++++++++++++++++---- test/gtest-port_test.cc | 41 +++----------- 2 files changed, 87 insertions(+), 42 deletions(-) diff --git a/include/gtest/internal/gtest-port.h b/include/gtest/internal/gtest-port.h index f6ed4d00..936dfd50 100644 --- a/include/gtest/internal/gtest-port.h +++ b/include/gtest/internal/gtest-port.h @@ -1838,8 +1838,9 @@ class ThreadWithParam : public ThreadWithParamBase { template class ThreadLocal : public ThreadLocalBase { public: - ThreadLocal() : default_() {} - explicit ThreadLocal(const T& value) : default_(value) {} + ThreadLocal() : default_factory_(new DefaultValueHolderFactory()) {} + explicit ThreadLocal(const T& value) + : default_factory_(new InstanceValueHolderFactory(value)) {} ~ThreadLocal() { ThreadLocalRegistry::OnThreadLocalDestroyed(this); } @@ -1853,6 +1854,7 @@ class ThreadLocal : public ThreadLocalBase { // knowing the type of T. class ValueHolder : public ThreadLocalValueHolderBase { public: + ValueHolder() : value_() {} explicit ValueHolder(const T& value) : value_(value) {} T* pointer() { return &value_; } @@ -1869,10 +1871,42 @@ class ThreadLocal : public ThreadLocalBase { } virtual ThreadLocalValueHolderBase* NewValueForCurrentThread() const { - return new ValueHolder(default_); + return default_factory_->MakeNewHolder(); } - const T default_; // The default value for each thread. + class ValueHolderFactory { + public: + ValueHolderFactory() {} + virtual ~ValueHolderFactory() {} + virtual ValueHolder* MakeNewHolder() const = 0; + + private: + GTEST_DISALLOW_COPY_AND_ASSIGN_(ValueHolderFactory); + }; + + class DefaultValueHolderFactory : public ValueHolderFactory { + public: + DefaultValueHolderFactory() {} + virtual ValueHolder* MakeNewHolder() const { return new ValueHolder(); } + + private: + GTEST_DISALLOW_COPY_AND_ASSIGN_(DefaultValueHolderFactory); + }; + + class InstanceValueHolderFactory : public ValueHolderFactory { + public: + explicit InstanceValueHolderFactory(const T& value) : value_(value) {} + virtual ValueHolder* MakeNewHolder() const { + return new ValueHolder(value_); + } + + private: + const T value_; // The value for each thread. + + GTEST_DISALLOW_COPY_AND_ASSIGN_(InstanceValueHolderFactory); + }; + + scoped_ptr default_factory_; GTEST_DISALLOW_COPY_AND_ASSIGN_(ThreadLocal); }; @@ -1993,10 +2027,11 @@ extern "C" inline void DeleteThreadLocalValue(void* value_holder) { template class ThreadLocal { public: - ThreadLocal() : key_(CreateKey()), - default_() {} - explicit ThreadLocal(const T& value) : key_(CreateKey()), - default_(value) {} + ThreadLocal() + : key_(CreateKey()), default_factory_(new DefaultValueHolderFactory()) {} + explicit ThreadLocal(const T& value) + : key_(CreateKey()), + default_factory_(new InstanceValueHolderFactory(value)) {} ~ThreadLocal() { // Destroys the managed object for the current thread, if any. @@ -2016,6 +2051,7 @@ class ThreadLocal { // Holds a value of type T. class ValueHolder : public ThreadLocalValueHolderBase { public: + ValueHolder() : value_() {} explicit ValueHolder(const T& value) : value_(value) {} T* pointer() { return &value_; } @@ -2041,15 +2077,47 @@ class ThreadLocal { return CheckedDowncastToActualType(holder)->pointer(); } - ValueHolder* const new_holder = new ValueHolder(default_); + ValueHolder* const new_holder = default_factory_->MakeNewHolder(); ThreadLocalValueHolderBase* const holder_base = new_holder; GTEST_CHECK_POSIX_SUCCESS_(pthread_setspecific(key_, holder_base)); return new_holder->pointer(); } + class ValueHolderFactory { + public: + ValueHolderFactory() {} + virtual ~ValueHolderFactory() {} + virtual ValueHolder* MakeNewHolder() const = 0; + + private: + GTEST_DISALLOW_COPY_AND_ASSIGN_(ValueHolderFactory); + }; + + class DefaultValueHolderFactory : public ValueHolderFactory { + public: + DefaultValueHolderFactory() {} + virtual ValueHolder* MakeNewHolder() const { return new ValueHolder(); } + + private: + GTEST_DISALLOW_COPY_AND_ASSIGN_(DefaultValueHolderFactory); + }; + + class InstanceValueHolderFactory : public ValueHolderFactory { + public: + explicit InstanceValueHolderFactory(const T& value) : value_(value) {} + virtual ValueHolder* MakeNewHolder() const { + return new ValueHolder(value_); + } + + private: + const T value_; // The value for each thread. + + GTEST_DISALLOW_COPY_AND_ASSIGN_(InstanceValueHolderFactory); + }; + // A key pthreads uses for looking up per-thread values. const pthread_key_t key_; - const T default_; // The default value for each thread. + scoped_ptr default_factory_; GTEST_DISALLOW_COPY_AND_ASSIGN_(ThreadLocal); }; diff --git a/test/gtest-port_test.cc b/test/gtest-port_test.cc index 937832bb..14418804 100644 --- a/test/gtest-port_test.cc +++ b/test/gtest-port_test.cc @@ -1149,13 +1149,6 @@ TEST(ThreadLocalTest, ParameterizedConstructorSetsDefault) { EXPECT_STREQ("foo", result.c_str()); } -# if !GTEST_HAS_MUTEX_AND_THREAD_LOCAL_ - -// Tests in this section depend on that Google Test's own ThreadLocal -// implementation stores a copy of the default value shared by all -// threads. We don't want to test this for an external implementation received -// through GTEST_HAS_MUTEX_AND_THREAD_LOCAL_. - // Keeps track of whether of destructors being called on instances of // DestructorTracker. On Windows, waits for the destructor call reports. class DestructorCall { @@ -1240,25 +1233,18 @@ TEST(ThreadLocalTest, DestroysManagedObjectForOwnThreadWhenDying) { DestructorCall::ResetList(); { - // The next line default constructs a DestructorTracker object as - // the default value of objects managed by thread_local_tracker. ThreadLocal thread_local_tracker; - ASSERT_EQ(1U, DestructorCall::List().size()); - ASSERT_FALSE(DestructorCall::List()[0]->CheckDestroyed()); + ASSERT_EQ(0U, DestructorCall::List().size()); // This creates another DestructorTracker object for the main thread. thread_local_tracker.get(); - ASSERT_EQ(2U, DestructorCall::List().size()); + ASSERT_EQ(1U, DestructorCall::List().size()); ASSERT_FALSE(DestructorCall::List()[0]->CheckDestroyed()); - ASSERT_FALSE(DestructorCall::List()[1]->CheckDestroyed()); } - // Now thread_local_tracker has died. It should have destroyed both the - // default value shared by all threads and the value for the main - // thread. - ASSERT_EQ(2U, DestructorCall::List().size()); + // Now thread_local_tracker has died. + ASSERT_EQ(1U, DestructorCall::List().size()); EXPECT_TRUE(DestructorCall::List()[0]->CheckDestroyed()); - EXPECT_TRUE(DestructorCall::List()[1]->CheckDestroyed()); DestructorCall::ResetList(); } @@ -1269,35 +1255,26 @@ TEST(ThreadLocalTest, DestroysManagedObjectAtThreadExit) { DestructorCall::ResetList(); { - // The next line default constructs a DestructorTracker object as - // the default value of objects managed by thread_local_tracker. ThreadLocal thread_local_tracker; - ASSERT_EQ(1U, DestructorCall::List().size()); - ASSERT_FALSE(DestructorCall::List()[0]->CheckDestroyed()); + ASSERT_EQ(0U, DestructorCall::List().size()); // This creates another DestructorTracker object in the new thread. ThreadWithParam thread( &CallThreadLocalGet, &thread_local_tracker, NULL); thread.Join(); - // The thread has exited, and we should have another DestroyedTracker + // The thread has exited, and we should have a DestroyedTracker // instance created for it. But it may not have been destroyed yet. - // The instance for the main thread should still persist. - ASSERT_EQ(2U, DestructorCall::List().size()); - ASSERT_FALSE(DestructorCall::List()[0]->CheckDestroyed()); + ASSERT_EQ(1U, DestructorCall::List().size()); } - // The thread has exited and thread_local_tracker has died. The default - // value should have been destroyed too. - ASSERT_EQ(2U, DestructorCall::List().size()); + // The thread has exited and thread_local_tracker has died. + ASSERT_EQ(1U, DestructorCall::List().size()); EXPECT_TRUE(DestructorCall::List()[0]->CheckDestroyed()); - EXPECT_TRUE(DestructorCall::List()[1]->CheckDestroyed()); DestructorCall::ResetList(); } -# endif // !GTEST_HAS_MUTEX_AND_THREAD_LOCAL_ - TEST(ThreadLocalTest, ThreadLocalMutationsAffectOnlyCurrentThread) { ThreadLocal thread_local_string; thread_local_string.set("Foo");