1365824 - Remove STORE_SEQUENCER

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Nicholas Nethercote [inactive]]

It's a software memory barrier, and not a very strong one. If the values it is
protecting are Atomic, that provides a stronger hardware memory barrier.

This patch removes it, and changes one of the values it was protecting from
|volatile| to Atomic. (The other value it was protecting was already Atomic.)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove STORE_SEQUENCER

froydnj: we talked about this a while back, and you said that you thought
STORE_SEQUENCER was adding zero value.

mstange, jseward: I'm asking for feedback in case you have any additional
thoughts.

Comment 2

[Julian Seward [:jseward]]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> This patch removes it, and changes one of the values it was
> protecting from |volatile| to Atomic.

Yes, this seems to me to be the right thing to do.

STORE_SEQUENCER appears to confuse "inhibit reordering by the
compiler" (__GNUC__) with "inhibit reordering by the hardware"
(__arch64__) or both (_MSC_VER, __INTEL_COMPILER) or
bizarre-kernel-call (__arm__).

Also, it doesn't seem to me to make much sense to have a store
fence in the writing thread without also having load fences for
the reading thread(s), which is currently the situation.

The only downside that I can see is that using atomics might be
more expensive than using store- and load- fences consistently.
But I'm just guessing here.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8868839 [details] [diff] [review]
Remove STORE_SEQUENCER

Review of attachment 8868839 [details] [diff] [review]:
-----------------------------------------------------------------

Three people agree, we must have used Atomic<> correctly!

::: tools/profiler/core/ProfilerMarker.h
@@ +146,5 @@
>    void insert(T* aElement)
>    {
>      MOZ_ASSERT(aElement);
>  
>      mSignalLock = true;

I am not going to ask too many questions about whether we can ever have a ProfilerSignalSafeLinkedList* referenced from two different threads, and we're racing on one thread insert()'ing and one thread calling the destructor...I mean, why even have this check otherwise...

Comment 4

[Nicholas Nethercote [inactive]]

> I am not going to ask too many questions about whether we can ever have a
> ProfilerSignalSafeLinkedList* referenced from two different threads, and
> we're racing on one thread insert()'ing and one thread calling the
> destructor...I mean, why even have this check otherwise...

I admit I have not looked closely at how this data structure works. Currently it's accessed in a single-threaded way -- the global mutex that I added to the profiler is always locked for accesses -- but that will have to change if we do bug 1347274.

Comment 5

[Nicholas Nethercote [inactive]]

> I admit I have not looked closely at how this data structure works.
> Currently it's accessed in a single-threaded way -- the global mutex that I
> added to the profiler is always locked for accesses -- but that will have to
> change if we do bug 1347274.

I just took a closer look. It's totally dodgy -- without the global mutex protecting it, it's possible to end up iterating over the list on one thread while inserting on another thread, because the boolean isn't sufficient protection. Sigh.

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37b33385378f97d52b38dfb140ca2be2ec9115f3
Bug 1365824 - Remove STORE_SEQUENCER. r=froydnj.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37b33385378f