Remove use of |volatile| from ProfileEntry

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
These annotations aren't doing anything useful. The important thing with
the PseudoStack is that, during pushes, the stack pointer incrementing happens
after the new entry is written, and this is ensured by the stack pointer being
Atomic.

The patch also improves the comments on PseudoStack.
(Assignee)

Comment 1

2 years ago
Four reviews may be overkill, but hey, it's multi-threaded programming.

BTW, my comments in this bug and patch exude confidence, but please don't
be fooled; I may have made mistakes.
Attachment #8873741 - Flags: review?(shu)
Attachment #8873741 - Flags: review?(nfroyd)
Attachment #8873741 - Flags: review?(mstange)
Attachment #8873741 - Flags: review?(jseward)
It looks to me as if the patch will work, sure.  But I would like to ask, was
there any consideration given to using store and load fences to ensure correct
sequencing?  I assume that the atomic will have the same effect, but what I'm
unclear about is whether that is its only purpose, or whether you also need
|stackPointer| to be updated atomically too.

I ask this because the (little) lockless programming I have seen leads me to
the impression that if all one requires is to ensure memory ordering, then
memory fences are the cheapest way to do that.
(Assignee)

Comment 3

2 years ago
|stackPointer| does need to be updated atomically.
Comment on attachment 8873741 [details] [diff] [review]
Remove use of |volatile| from ProfileEntry

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

Looks OK to me.

::: js/public/ProfilingStack.h
@@ +200,5 @@
> +//   entry visible to the sampler thread -- only after the new entry has been
> +//   fully written. The stack pointer is Atomic<> to ensure the incrementing is
> +//   not reordered before the writes.
> +//
> +// - When popping and old entry, the only operation is the decrementing of the

s/and/an
Attachment #8873741 - Flags: review?(jseward) → review+
Comment on attachment 8873741 [details] [diff] [review]
Remove use of |volatile| from ProfileEntry

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

r+ with the requested comment changes.

To the best of my knowledge this should work correctly.

::: js/public/ProfilingStack.h
@@ +198,5 @@
> +//
> +// - When pushing a new entry, we increment the stack pointer -- making the new
> +//   entry visible to the sampler thread -- only after the new entry has been
> +//   fully written. The stack pointer is Atomic<> to ensure the incrementing is
> +//   not reordered before the writes.

I think this comment should explicitly call out that this mozilla::Atomic value uses the default memory ordering "SequentiallyConsistent", which is what gives us the guarantee that we need. On its own, a value being atomic only means that it can't be observed in half-written states, it doesn't say much about ordering.
At least that's my understanding.

@@ +260,5 @@
>  
>      // This may exceed MaxEntries, so instead use the stackSize() method to
>      // determine the number of valid samples in entries. When this is less
>      // than MaxEntries, it refers to the first free entry past the top of the
>      // in-use stack (i.e. entries[stackPointer - 1] is the top stack entry).

Should probably also mention here that Atomic<uint32_t> means we get the memory ordering SequentiallyConsistent.
Attachment #8873741 - Flags: review?(mstange) → review+
(In reply to Julian Seward [:jseward] from comment #2)
> It looks to me as if the patch will work, sure.  But I would like to ask, was
> there any consideration given to using store and load fences to ensure
> correct
> sequencing?  I assume that the atomic will have the same effect

It has the same effect because Atomic<uint32_t> is equivalent to Atomic<uint32_t, SequentiallyConsistent>, which will cause us to insert fences as necessary.

Updated

2 years ago
Attachment #8873741 - Flags: review?(shu) → review+
(Assignee)

Comment 7

2 years ago
I made the SequentiallyConsistent semantics explicit on the declaration of
stackPointer_. I also mentioned those semantics in the comment.

froydnj is the only one left to review! :)
Attachment #8874271 - Flags: review?(nfroyd)
(Assignee)

Updated

2 years ago
Attachment #8873741 - Attachment is obsolete: true
Attachment #8873741 - Flags: review?(nfroyd)
Comment on attachment 8874271 [details] [diff] [review]
Remove use of |volatile| from ProfileEntry

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

Death to volatile.
Attachment #8874271 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a325d838180b07692bbb0e04094ad5e1dc06cca9
Bug 1369644 - Remove use of |volatile| from ProfileEntry. r=mstange,shu,jseward,froydnj.
https://hg.mozilla.org/mozilla-central/rev/a325d838180b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.