Proposal: use base::NoDestructor for leaky non-POD globals (and get rid of base::Singleton?)

[Daniel Cheng]

Currently, we have several ways to define non-POD globals. The many different ways (base::LazyInstance, base::Singleton, base::NoDestructor, CR_DEFINE_STATIC_LOCAL, and a function-local static) all support the same set of basic restrictions:

• the global should be lazily constructed
  
• the global must not generate global constructors or destructors.
  

To simplify the situation, I'd like to propose that we standardize on using base::NoDestructor for leaky non-POD globals.

Why use base::NoDestructor?

• unlike CR_DEFINE_STATIC_LOCAL or defining a function-local static that leaks the object, the object is stored inline. In theory, this should be more efficient since we don't need to deref a pointer to follow it to the heap block where it actually lives.
  
• unlike base::LazyInstance and base::Singleton, it relies on the built-in thread-safety of function-local statics in C++11. The synchronization logic in base::LazyInstance and base::Singleton has had bugs in the past, such as https://crrev.com/527445. Being able to remove this subtle code would be nice.
  Note: while most code in Chromium doesn't need to be thread-safe, we have thread-safe statics enabled globally, so we're already paying the cost globally. Perhaps compilers will provide a way to opt-out in the future, so we only pay this cost for things that really need to be thread-safe…
• unlike base::LazyInstance, it supports forwarding arguments to the type's constructor. This makes it easier to support more complex initialization.

What are the disadvantages of base::NoDestructor?

• The storage cost for an object allocated in base::NoDestructor is paid up front, since the storage is inline. In contrast, we don't pay the storage cost for CR_DEFINE_STATIC_LOCAL or a heap-allocated function-local static until the object is accessed. To mitigate this, we could conditionally heap-allocate for objects larger than a certain size (in fact, this is what Blink does).
  
• We may not actually trust the toolchain implementation to be free of things like priority-inversion bugs either.
  

For most things, I think getting rid of the extra pointer hop is a win. And if we need to tweak the allocation strategy to avoid putting too much data in bss, it's fairly simple to tweak later on.

Why get rid of base::Singleton?

The only thing it provides that the other helpers don't is an attempt to enforce that a given type is only instantiated once globally, but the only way to implement this in C++ encourages potential ODR violations.

Most uses can be replaced with base::NoDestructor; things that really need the current destroy-at-exit behavior can use base::LazyInstance::DestroyAtExit.

Thoughts?

Daniel

[maxm...@chromium.org]

Completely agree on removing LazyInstance, Singleton and the macro. In addition to the reasons listed, having so many home-grown solutions adds a lot of overhead to people new to the project.

[Avi Drissman]

I agree with consolidating on NoDestructor. The others are relics from when we couldn't rely on language features and had to roll things ourselves. In addition, having in the past struggled to understand the nuances of the different choices, I strongly agree with Max in removing all the others.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/da92dbda-8503-4468-9343-264a8d870679%40chromium.org.

[Nico Weber]

Sounds good to me too.

I'll point out that the atomic initializer we get on Windows for function-local statics is kind of large codesize-wise (I don't remember if that's MSVC-only or also true with clang-cl – probably true with the latter too for abi reasons (?)). I think scottmg@ started replacing some LazyInstances with just local statics, and iirc that increased code size. Maybe he remembers details. But that might be something to look out for.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAYFcOqaRS9Ty7ZyXDw_koG4kmetys2y729J6pUdx6oKhA%40mail.gmail.com.

[Gabriel Charette]

I agree to consolidate but want to raise a few things we should consider first : 

1) DestructorAtExit traits are actually useful in unit tests to ensure isolation between tests. This is important to avoid flakiness unless we strive for all of these instances to be stateless. For LazyTaskRunner we made it leak in production but always be recycled in tests, I think that's ideal.

2) Given how tricky priority inversions have been I wouldn't be surprised if the platform provided thread-safe initialization is worse than ours. Let's take it through the equivalent of LazyInstanceTest.PriorityInversionAtInitializationResolves on all platforms before we proceed.

Other than that I'm very excited with this change. Can we also make the constructor of NoDestructor constexpr? That will give a hint to the compiler that if the args are constexpr and T is constexpr constructible with those args, it can initialize this static at compile time (e.g. default initialization in particular). The spec doesn't enforce compile time initialization if the destination variable isn't constexpr but clang generally will do it when method/args are constexpr. That would address crbug.com/800760 (avoiding thread-safe init for variables that can be initialized at compile time).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAYFcOqaRS9Ty7ZyXDw_koG4kmetys2y729J6pUdx6oKhA%40mail.gmail.com.

[John Abd-El-Malek]

For the common case of globals used by a single thread and not needing arguments for constructors, having to wrap access through a method instead of declaring it globally is unfortunate. Can that be fixed somehow? If not, I prefer LazyInstance for that case.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LLKmhCXZVf3bjgbDPaKHbpCf8e%3D1CpJjEiWa54md9z8Tg%40mail.gmail.com.

[Gabriel Charette]

On Wed, Mar 28, 2018 at 12:55 PM John Abd-El-Malek <j...@chromium.org> wrote:

For the common case of globals used by a single thread and not needing arguments for constructors, having to wrap access through a method instead of declaring it globally is unfortunate. Can that be fixed somehow? If not, I prefer LazyInstance for that case.

I agree, I was actually hoping to rely on clang's [[clang::require_constant_initialization]] attribute for non-const globals that can be initialized at compile-time and hence wouldn't require any special class to avoid a static initializer. But it turns out the C++ spec sadly doesn't enforce compile-time initialization of variables that aren't constexpr even if the constructor and args are all constexpr (and the next best thing is local static variable -- global variable initialization is not lazy). Clang does it, but MSVC doesn't. We discussed this here.

Barring clang::require_constant_initialization, perhaps a viable alternative would be a macro that allows one line definition while using NoDestructor's thread-safe-laz-init-without-heap? Would also make it easy to migrate to something akin to clang::require_constant_initialization if it's ever more widely supported.

e.g.

#define LAZY_GLOBAL(var_type, var_name)       \

  struct {                                    \

    var_type& Get() {                         \

      static NoDestructor<var_type> instance; \

      return *instance;                       \

    }                                         \

  } var_name

(+ variadic args to enable arbitrary constructor params)

Then you can :

LAZY_GLOBAL(base::TimeTicks, my_time);

Foo(my_time.Get())

Maybe Get() isn't the best name for that method because it's a bit weird in l-value case:

my_time.Get() = other_timeticks;

but same idea with better method name...

[Daniel Cheng]

Unfortunately, I don't think there's any way to both support general initialization (like base::NoDestructor does) and also support lazy global construction. As gab says, perhaps we could add a macro for doing this, though it'd still require function call syntax to get the global =/

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALhVsw0a%3DryRs6b2oBgEO2af6kB4aGvt_UC1d-mhY6y5ajvdDw%40mail.gmail.com.

[Gabriel Charette]

On Wed, Mar 28, 2018 at 3:08 PM Daniel Cheng <dch...@chromium.org> wrote:

Unfortunately, I don't think there's any way to both support general initialization (like base::NoDestructor does) and also support lazy global construction. As gab says, perhaps we could add a macro for doing this, though it'd still require function call syntax to get the global =/

Actually, how about

#define LAZY_GLOBAL(var_type, var_name)       \

  struct {                                    \

    var_type& operator*() {                   \

      static NoDestructor<var_type> instance; \

      return *instance;                       \

    }                                         \

  } var_name

Then the accessor is a mere * ...?

[Ken Rockot]

Regardless of ergonomics it still seems unfortunate that this would generate unnecessarily thread-safe code, no?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LLOqXhTCppPVr5kr6sjODcxVQgbT1QsoGMJdtmTNWM3gQ%40mail.gmail.com.

[Daniel Cheng]

All of our variants for lazily constructed globals are thread-safe. There are no non-thread-safe versions anymore, since we enabled thread-safe statics (LazyInstance and Singleton have always been thread-safe).

Daniel

[Gabriel Charette]

Right, the only way to avoid thread-safe is [[clang::require_constant_initialization]] which we can't use yet.

[Joe Mason]

+1 to getting rid of base::Singleton eventually.

However next quarter I'll be upstreaming some code that's currently in an internal repository which uses base::Singleton, so I'd appreciate it if that's finished before actually getting rid of the class itself. That way this code can be updated as part of the general process of updating Chromium code to not use Singleton, instead of being an extra task I'll need to do before upstreaming it.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LLpcwF%2BRb9A4-5qUEFJAHbJj9i7XRGYvaxZmAWKK_m1ug%40mail.gmail.com.