Cheaper thread-safe atomic initialization of static references.

Cheaper thread-safe atomic initialization of static references.

Introduce AtomicallyInitializedStaticReference(Type, name, initializer)
for creating static singletons in a thread-safe manner. Reduce overhead
compared to the previous AtomicallyInitializedStatic() by adding a
double-checked locking scheme around the initialization.

Adopt its use across Blink.

R=jochen,kinuko,haraken,jyasskin
BUG=447370
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188774

[sof, 2015-01-21 15:03:18]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, jamesr@chromium.org, jochen@chromium.org,
jyasskin@chromium.org, kinuko@chromium.org, tkent@chromium.org

[sof, 2015-01-21 15:03:19]

Please take a look.

A longer list of reviewers, in case people are too busy or only want to review a
subset. Or not.

[jochen (gone - plz use gerrit), 2015-01-21 15:15:23]

lgtm

[kinuko, 2015-01-21 15:21:18]

lgtm2!

[haraken, 2015-01-21 15:23:33]

LGTM

https://codereview.chromium.org/794223003/diff/1/Source/wtf/ArrayBufferConten...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/794223003/diff/1/Source/wtf/ArrayBufferConten...
Source/wtf/ArrayBufferContents.h:64: m_deallocationObserver = &observer;

Do we need m_deallocationObserver? I guess m_deallocationObserver is always
ArrayBufferDeallocationObserver::instance().

https://codereview.chromium.org/794223003/diff/1/Source/wtf/Threading.h
File Source/wtf/Threading.h (right):

https://codereview.chromium.org/794223003/diff/1/Source/wtf/Threading.h#newco...
Source/wtf/Threading.h:42: static WTF::RemoveConst<T>::Type* name##_pointer =
nullptr;                          \

_pointer => Pointer

[sof, 2015-01-21 15:51:38]

https://codereview.chromium.org/794223003/diff/1/Source/wtf/ArrayBufferConten...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/794223003/diff/1/Source/wtf/ArrayBufferConten...
Source/wtf/ArrayBufferContents.h:64: m_deallocationObserver = &observer;
On 2015/01/21 15:23:33, haraken wrote:
> 
> Do we need m_deallocationObserver? I guess m_deallocationObserver is always
> ArrayBufferDeallocationObserver::instance().

It looks that way; is having a more general interface worthwhile? I can't say,
but I can handle that separately should it not be.

(+Cc: yukishiino)

https://codereview.chromium.org/794223003/diff/1/Source/wtf/Threading.h
File Source/wtf/Threading.h (right):

https://codereview.chromium.org/794223003/diff/1/Source/wtf/Threading.h#newco...
Source/wtf/Threading.h:42: static WTF::RemoveConst<T>::Type* name##_pointer =
nullptr;                          \
On 2015/01/21 15:23:33, haraken wrote:
> 
> _pointer => Pointer

Thanks, switched.

[Jeffrey Yasskin, 2015-01-21 16:53:20]

The reinterpret_casts are undefined behavior. Make the pointer a void* and car
the assignment to a reference instead.

[haraken, 2015-01-21 16:59:17]

https://codereview.chromium.org/794223003/diff/1/Source/wtf/ArrayBufferConten...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/794223003/diff/1/Source/wtf/ArrayBufferConten...
Source/wtf/ArrayBufferContents.h:64: m_deallocationObserver = &observer;
On 2015/01/21 15:51:38, sof wrote:
> On 2015/01/21 15:23:33, haraken wrote:
> > 
> > Do we need m_deallocationObserver? I guess m_deallocationObserver is always
> > ArrayBufferDeallocationObserver::instance().
> 
> It looks that way; is having a more general interface worthwhile? I can't say,
> but I can handle that separately should it not be.
> 
> (+Cc: yukishiino)

You can handle this in a follow-up either way :)

[sof, 2015-01-21 19:23:11]

On 2015/01/21 16:53:20, Jeffrey Yasskin wrote:
> The reinterpret_casts are undefined behavior. Make the pointer a void* and car
> the assignment to a reference instead.

Thanks, something along the lines of
https://codereview.chromium.org/794223003/patch/40001/50039 ?

[Jeffrey Yasskin, 2015-01-21 19:36:07]

https://codereview.chromium.org/794223003/diff/40001/Source/wtf/Threading.h
File Source/wtf/Threading.h (right):

https://codereview.chromium.org/794223003/diff/40001/Source/wtf/Threading.h#n...
Source/wtf/Threading.h:45: WTF::releaseStore(&name##Pointer, initializer);      
 \
It probably makes sense to use:

if (!WTF::acquireLoad(&name##Pointer)) {
  T* initializerResult = initializer;
  WTF::releaseStore(&name##Pointer, initializerResult);
}

to make sure that the initializer returns the right type.

Otherwise, this version looks great. Thanks for implementing this!

[sof, 2015-01-21 20:42:54]

Thanks for the quick reviews.

https://codereview.chromium.org/794223003/diff/40001/Source/wtf/Threading.h
File Source/wtf/Threading.h (right):

https://codereview.chromium.org/794223003/diff/40001/Source/wtf/Threading.h#n...
Source/wtf/Threading.h:45: WTF::releaseStore(&name##Pointer, initializer);      
 \
On 2015/01/21 19:36:06, Jeffrey Yasskin wrote:
> It probably makes sense to use:
> 
> if (!WTF::acquireLoad(&name##Pointer)) {
>   T* initializerResult = initializer;
>   WTF::releaseStore(&name##Pointer, initializerResult);
> }
> 
> to make sure that the initializer returns the right type.
> 
> Otherwise, this version looks great. Thanks for implementing this!

The extra type check is definitely worth it; added.

[sof, 2015-01-21 20:43:04]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-01-21 20:43:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/794223003/60001

[Jeffrey Yasskin, 2015-01-21 20:59:48]

lgtm

[commit-bot: I haz the power, 2015-01-21 21:59:54]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188774

[Yuki, 2015-01-22 04:27:38]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org

[Yuki, 2015-01-22 04:27:39]

https://codereview.chromium.org/794223003/diff/1/Source/wtf/ArrayBufferConten...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/794223003/diff/1/Source/wtf/ArrayBufferConten...
Source/wtf/ArrayBufferContents.h:64: m_deallocationObserver = &observer;
On 2015/01/21 16:59:17, haraken wrote:
> On 2015/01/21 15:51:38, sof wrote:
> > On 2015/01/21 15:23:33, haraken wrote:
> > > 
> > > Do we need m_deallocationObserver? I guess m_deallocationObserver is
always
> > > ArrayBufferDeallocationObserver::instance().
> > 
> > It looks that way; is having a more general interface worthwhile? I can't
say,
> > but I can handle that separately should it not be.
> > 
> > (+Cc: yukishiino)
> 
> You can handle this in a follow-up either way :)

m_deallocationObserver must always be
**DOM**ArrayBufferDeallocationObserver::instance(), not
ArrayBufferDeallocationObserver::instance().

Since wtf/ArrayBufferContents cannot (directly) depend on
core/dom/DOMArrayBufferDeallocationObserver, we need m_deallocationObserver.

[Yuki, 2015-01-22 04:35:20]

https://codereview.chromium.org/794223003/diff/1/Source/wtf/ArrayBufferConten...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/794223003/diff/1/Source/wtf/ArrayBufferConten...
Source/wtf/ArrayBufferContents.h:64: m_deallocationObserver = &observer;
On 2015/01/22 04:27:39, Yuki wrote:
> On 2015/01/21 16:59:17, haraken wrote:
> > On 2015/01/21 15:51:38, sof wrote:
> > > On 2015/01/21 15:23:33, haraken wrote:
> > > > 
> > > > Do we need m_deallocationObserver? I guess m_deallocationObserver is
> always
> > > > ArrayBufferDeallocationObserver::instance().
> > > 
> > > It looks that way; is having a more general interface worthwhile? I can't
> say,
> > > but I can handle that separately should it not be.
> > > 
> > > (+Cc: yukishiino)
> > 
> > You can handle this in a follow-up either way :)
> 
> m_deallocationObserver must always be
> **DOM**ArrayBufferDeallocationObserver::instance(), not
> ArrayBufferDeallocationObserver::instance().
> 
> Since wtf/ArrayBufferContents cannot (directly) depend on
> core/dom/DOMArrayBufferDeallocationObserver, we need m_deallocationObserver.

Additional note:
We've made all the DOM objects inherit from ScriptWrappable, so ArrayBuffer.idl
(implemented as core/dom/DOMArrayBuffer) as well.  We no longer need
wtf/ArrayBuffer, so we're planning to remove old wtf/ArrayBuffer family except
for ArrayBufferContents.

wtf/ArrayBufferContents is not a DOM object, and some code (especially
platform/graphics/) need a class which represents a memory block which can be
efficiently converted to DOMArrayBuffer.  So my plan is keep
wtf/ArrayBufferContents as is in wtf/, and let platform/ code use it.

Then, m_deallocationObserver is useful to pass
core/dom/DOMArrayBufferDeallocationObserver to wtf/ArrayBufferContents.

[Noel Gordon, 2015-01-24 01:20:05]

noel@chromium.org changed reviewers:
+ noel@chromium.org

[Noel Gordon, 2015-01-24 01:20:06]

Nice patch: some minor questions.

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h
File Source/wtf/Threading.h (right):

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h#n...
Source/wtf/Threading.h:40: #define AtomicallyInitializedStaticReference(T, name,
initializer)      \
To "reduce overhead" relative to what?

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h#n...
Source/wtf/Threading.h:43: if (!WTF::acquireLoad(&name##Pointer)) {             
              \
Blink prefers the early return. Any reason not to use it here?

[sof, 2015-01-24 06:20:06]

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h
File Source/wtf/Threading.h (right):

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h#n...
Source/wtf/Threading.h:40: #define AtomicallyInitializedStaticReference(T, name,
initializer)      \
On 2015/01/24 01:20:06, noel gordon wrote:
> To "reduce overhead" relative to what?

To what's on the left here in the diff? :) Doesn't the use of "double-checked
locking" give enough context for someone wanting to understand motivation?

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h#n...
Source/wtf/Threading.h:43: if (!WTF::acquireLoad(&name##Pointer)) {             
              \
On 2015/01/24 01:20:06, noel gordon wrote:
> Blink prefers the early return. Any reason not to use it here?

You have to bind the reference, and at the scope the macro is used, so I don't
see the alternative you have in mind to evaluate if that is clearer.

[Noel Gordon, 2015-01-24 12:08:24]

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h
File Source/wtf/Threading.h (right):

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h#n...
Source/wtf/Threading.h:40: #define AtomicallyInitializedStaticReference(T, name,
initializer)      \
On 2015/01/24 06:20:06, sof wrote:
> On 2015/01/24 01:20:06, noel gordon wrote:
> > To "reduce overhead" relative to what?
> 
> To what's on the left here in the diff? :) Doesn't the use of "double-checked
> locking" give enough context for someone wanting to understand motivation?

Think _we_ understand the motivation :) but other developers reading this code
are not necessarily looking at a diff, or might not know what double-checked
locking is, or how it reduces the overhead relative to some other unspecified
thing (which is perhaps the cost of locking a mutex, the comment doesn't really
say). Perhaps in this instance, the comment at diff-left was somewhat better.

imho ... change descriptions are the best place to say such things, at least in
the Blink project. Developers can 'git blame' to find the relevant descriptions
to read and, given well-written change descriptions that explain the "why", that
helps new developers gain a deeper understanding of our code.

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h#n...
Source/wtf/Threading.h:43: if (!WTF::acquireLoad(&name##Pointer)) {             
              \
On 2015/01/24 06:20:06, sof wrote:
> On 2015/01/24 01:20:06, noel gordon wrote:
> > Blink prefers the early return. Any reason not to use it here?
> 
> You have to bind the reference

Yes I see your point, @51 it seems, so ok.

[sof, 2015-01-24 12:40:05]

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h
File Source/wtf/Threading.h (right):

https://codereview.chromium.org/794223003/diff/60001/Source/wtf/Threading.h#n...
Source/wtf/Threading.h:40: #define AtomicallyInitializedStaticReference(T, name,
initializer)      \
On 2015/01/24 12:08:24, noel gordon wrote:
> On 2015/01/24 06:20:06, sof wrote:
> > On 2015/01/24 01:20:06, noel gordon wrote:
> > > To "reduce overhead" relative to what?
> > 
> > To what's on the left here in the diff? :) Doesn't the use of
"double-checked
> > locking" give enough context for someone wanting to understand motivation?
> 
> Think _we_ understand the motivation :) but other developers reading this code
> are not necessarily looking at a diff, or might not know what double-checked
> locking is, or how it reduces the overhead relative to some other unspecified
> thing (which is perhaps the cost of locking a mutex, the comment doesn't
really
> say). Perhaps in this instance, the comment at diff-left was somewhat better.
> 
> imho ... change descriptions are the best place to say such things, at least
in
> the Blink project. Developers can 'git blame' to find the relevant
descriptions
> to read and, given well-written change descriptions that explain the "why",
that
> helps new developers gain a deeper understanding of our code.

Sounds sensible; doesn't the description here explain in a bit more detail?

[Noel Gordon, 2015-01-24 13:03:14]

On 2015/01/24 12:40:05, sof wrote:
> Sounds sensible; doesn't the description here explain in a bit more detail?

It cetainly does. You wrote a very good change decription here.

[Noel Gordon, 2015-01-24 13:04:52]

/certainly even.

[sof, 2015-01-24 13:22:24]

On 2015/01/24 13:03:14, noel gordon wrote:
> On 2015/01/24 12:40:05, sof wrote:
> > Sounds sensible; doesn't the description here explain in a bit more detail?
> 
> It cetainly does. You wrote a very good change decription here.

Just trying to explicate jyasskin's original work in this case. But I fully
agree about the value of commit messages.

[Noel Gordon, 2015-01-24 14:18:45]

LGTM

Understood. Good to see this change in Blink. Thank you for doing this work.