Closed Bug 1365824 Opened 2 years ago Closed 2 years ago

Remove STORE_SEQUENCER

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(1 file)

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.)
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.
Attachment #8868839 - Flags: review?(nfroyd)
Attachment #8868839 - Flags: feedback?(mstange)
Attachment #8868839 - Flags: feedback?(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.
Attachment #8868839 - Flags: feedback?(jseward) → feedback+
Attachment #8868839 - Flags: feedback?(mstange) → feedback+
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...
Attachment #8868839 - Flags: review?(nfroyd) → review+
> 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.
> 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.
https://hg.mozilla.org/mozilla-central/rev/37b33385378f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.