Tweak GetOrCreateLazyPointer's template logic to enable more code folding.

This CL saves 27KB in chrome.dll and 5KB in chrome_child.dll.

https://chromium-review.googlesource.com/868013 had regressed size by
9KB and was caught as a performance regression. This CL should recover
this 3X.

Also removed an extra atomic operation in the simple case (used to
atomic-load for check and atomic-load again for return, now only
atomic-loads once).

Also moved GetOrCreateLazyPointer() to a subtle:: namespace (now that
calling it is tricky), also resolving recurring confusion about whether
Needs/CompleteLazyInstance() are invoked outside of it (they're
not, they're strictly helpers).

R=fdoray@chromium.org, thakis@chromium.org

Bug: 804034
Change-Id: I8de94fe742a8f9a68d4a66e0202a6fb276843af9
Reviewed-on: https://chromium-review.googlesource.com/883364Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532013}

base/lazy_instance.h

@@ -161,9 +161,9 @@ class LazyInstance {
       ThreadRestrictions::AssertSingletonAllowed();
 #endif
 
-    return static_cast<Type*>(internal::GetOrCreateLazyPointer(
-        &private_instance_, [this]() { return Traits::New(private_buf_); },
-        Traits::kRegisterOnExit ? OnExit : nullptr, this));
+    return subtle::GetOrCreateLazyPointer(
+        &private_instance_, &Traits::New, private_buf_,
+        Traits::kRegisterOnExit ? OnExit : nullptr, this);
   }
 
   // Returns true if the lazy instance has been created.  Unlike Get() and

base/lazy_instance_helpers.h

@@ -32,22 +32,36 @@ BASE_EXPORT void CompleteLazyInstance(subtle::AtomicWord* state,
                                       void (*destructor)(void*),
                                       void* destructor_arg);
 
-// If |state| is uninitialized (zero), constructs a value using |creator_func|,
-// stores it into |state| and registers |destructor| to be called with
-// |destructor_arg| as argument when the current AtExitManager goes out of
-// scope. Then, returns the value stored in |state|. It is safe to have
+}  // namespace internal
+
+namespace subtle {
+
+// If |state| is uninitialized (zero), constructs a value using
+// |creator_func(creator_arg)|, stores it into |state| and registers
+// |destructor(destructor_arg)| to be called when the current AtExitManager goes
+// out of scope. Then, returns the value stored in |state|. It is safe to have
 // concurrent calls to this function with the same |state|. |creator_func| may
 // return nullptr if it doesn't want to create an instance anymore (e.g. on
 // shutdown), it is from then on required to return nullptr to all callers (ref.
-// StaticMemorySingletonTraits). Callers need to synchronize before
-// |creator_func| may return a non-null instance again (ref.
+// StaticMemorySingletonTraits). In that case, callers need to synchronize
+// before |creator_func| may return a non-null instance again (ref.
 // StaticMemorySingletonTraits::ResurectForTesting()).
-template <typename CreatorFunc>
-void* GetOrCreateLazyPointer(subtle::AtomicWord* state,
-                             const CreatorFunc& creator_func,
+// Implementation note on |creator_func/creator_arg|. It makes for ugly adapters
+// but it avoids redundant template instantiations (e.g. saves 27KB in
+// chrome.dll) because linker is able to fold these for multiple Types but
+// couldn't with the more advanced CreatorFunc template type which in turn
+// improves code locality (and application startup) -- ref.
+// https://chromium-review.googlesource.com/c/chromium/src/+/530984/5/base/lazy_instance.h#140,
+// worsened by https://chromium-review.googlesource.com/c/chromium/src/+/868013
+// and caught then as https://crbug.com/804034.
+template <typename Type>
+Type* GetOrCreateLazyPointer(subtle::AtomicWord* state,
+                             Type* (*creator_func)(void*),
+                             void* creator_arg,
                              void (*destructor)(void*),
                              void* destructor_arg) {
   DCHECK(state);
+  DCHECK(creator_func);
 
   // If any bit in the created mask is true, the instance has already been
   // fully constructed.
@@ -60,18 +74,28 @@ void* GetOrCreateLazyPointer(subtle::AtomicWord* state,
   // has acquire memory ordering as a thread which sees |state| > creating needs
   // to acquire visibility over the associated data. Pairing Release_Store is in
   // CompleteLazyInstance().
-  if (!(subtle::Acquire_Load(state) & kLazyInstanceCreatedMask) &&
-      NeedsLazyInstance(state)) {
-    // This thread won the race and is now responsible for creating the instance
-    // and storing it back into |state|.
-    subtle::AtomicWord instance =
-        reinterpret_cast<subtle::AtomicWord>(creator_func());
-    CompleteLazyInstance(state, instance, destructor, destructor_arg);
+  subtle::AtomicWord instance = subtle::Acquire_Load(state);
+  if (!(instance & kLazyInstanceCreatedMask)) {
+    if (internal::NeedsLazyInstance(state)) {
+      // This thread won the race and is now responsible for creating the
+      // instance and storing it back into |state|.
+      instance =
+          reinterpret_cast<subtle::AtomicWord>((*creator_func)(creator_arg));
+      internal::CompleteLazyInstance(state, instance, destructor,
+                                     destructor_arg);
+    } else {
+      // This thread lost the race but now has visibility over the constructed
+      // instance (NeedsLazyInstance() doesn't return until the constructing
+      // thread releases the instance via CompleteLazyInstance()).
+      instance = subtle::Acquire_Load(state);
+      DCHECK(instance & kLazyInstanceCreatedMask);
+    }
   }
-  return reinterpret_cast<void*>(subtle::NoBarrier_Load(state));
+  return reinterpret_cast<Type*>(instance);
 }
 
-}  // namespace internal
+}  // namespace subtle
+
 }  // namespace base
 
 #endif  // BASE_LAZY_INSTANCE_INTERNAL_H_

base/memory/singleton.h

@@ -240,11 +240,15 @@ class Singleton {
       ThreadRestrictions::AssertSingletonAllowed();
 #endif
 
-    return static_cast<Type*>(internal::GetOrCreateLazyPointer(
-        &instance_, &Traits::New, Traits::kRegisterAtExit ? OnExit : nullptr,
-        nullptr));
+    return subtle::GetOrCreateLazyPointer(
+        &instance_, &CreatorFunc, nullptr,
+        Traits::kRegisterAtExit ? OnExit : nullptr, nullptr);
   }
 
+  // Internal method used as an adaptor for GetOrCreateLazyPointer(). Do not use
+  // outside of that use case.
+  static Type* CreatorFunc(void* /* creator_arg*/) { return Traits::New(); }
+
   // Adapter function for use with AtExit().  This should be called single
   // threaded, so don't use atomic operations.
   // Calling OnExit while singleton is in use by other threads is a mistake.

base/task_scheduler/lazy_task_runner.cc

@@ -62,30 +62,36 @@ LazyTaskRunner<SingleThreadTaskRunner, true>::Create() {
 }
 #endif
 
+// static
+template <typename TaskRunnerType, bool com_sta>
+TaskRunnerType* LazyTaskRunner<TaskRunnerType, com_sta>::CreateRaw(
+    void* void_self) {
+  auto self =
+      reinterpret_cast<LazyTaskRunner<TaskRunnerType, com_sta>*>(void_self);
+
+  scoped_refptr<TaskRunnerType> task_runner = self->Create();
+
+  // Acquire a reference to the TaskRunner. The reference will either
+  // never be released or be released in Reset(). The reference is not
+  // managed by a scoped_refptr because adding a scoped_refptr member to
+  // LazyTaskRunner would prevent its static initialization.
+  task_runner->AddRef();
+
+  // Reset this instance when the current
+  // ScopedLazyTaskRunnerListForTesting is destroyed, if any.
+  if (g_scoped_lazy_task_runner_list_for_testing) {
+    g_scoped_lazy_task_runner_list_for_testing->AddCallback(BindOnce(
+        &LazyTaskRunner<TaskRunnerType, com_sta>::Reset, Unretained(self)));
+  }
+
+  return task_runner.get();
+}
+
 template <typename TaskRunnerType, bool com_sta>
 scoped_refptr<TaskRunnerType> LazyTaskRunner<TaskRunnerType, com_sta>::Get() {
-  return WrapRefCounted(static_cast<TaskRunnerType*>(GetOrCreateLazyPointer(
-      &state_,
-      [this]() {
-        scoped_refptr<TaskRunnerType> task_runner = Create();
-
-        // Acquire a reference to the TaskRunner. The reference will either
-        // never be released or be released in Reset(). The reference is not
-        // managed by a scoped_refptr because adding a scoped_refptr member to
-        // LazyTaskRunner would prevent its static initialization.
-        task_runner->AddRef();
-
-        // Reset this instance when the current
-        // ScopedLazyTaskRunnerListForTesting is destroyed, if any.
-        if (g_scoped_lazy_task_runner_list_for_testing) {
-          g_scoped_lazy_task_runner_list_for_testing->AddCallback(
-              BindOnce(&LazyTaskRunner<TaskRunnerType, com_sta>::Reset,
-                       Unretained(this)));
-        }
-
-        return task_runner.get();
-      },
-      nullptr, nullptr)));
+  return WrapRefCounted(subtle::GetOrCreateLazyPointer(
+      &state_, &LazyTaskRunner<TaskRunnerType, com_sta>::CreateRaw,
+      reinterpret_cast<void*>(this), nullptr, nullptr));
 }
 
 template class LazyTaskRunner<SequencedTaskRunner, false>;

base/task_scheduler/lazy_task_runner.h

@@ -158,6 +158,12 @@ class BASE_EXPORT LazyTaskRunner {
   // Creates and returns a new TaskRunner.
   scoped_refptr<TaskRunnerType> Create();
 
+  // Creates a new TaskRunner via Create(), adds an explicit ref to it, and
+  // returns it raw. Used as an adapter for lazy instance helpers. Static and
+  // takes |this| as an explicit param to match the void* signature of
+  // GetOrCreateLazyPointer().
+  static TaskRunnerType* CreateRaw(void* void_self);
+
   // TaskTraits to create the TaskRunner.
   const TaskTraits traits_;