1502207 - Avoid contention when recording atomic accesses

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

When atomic accesses are performed in a recording/replaying process, they are wrapped in a recorded mutex in order to ensure the atomic accesses happen in the same order while replaying that they did while recording.  This is simple but it's also slow: accessing the mutex has a lot of overhead, but more importantly this introduces a lot of artificial contention between threads.  There isn't any need to replay atomic accesses in the exact order they happened while recording; it is sufficient to ensure that accesses on the *same* atomic happen while replaying as while recording.  I.e. conceptually each atomic value wraps a lock around its accesses, but instead of there being a single lock for all values there could be a different lock for each value.

The attached patch picks a middle ground between using a single lock and using a separate lock for every atomic.  A fixed size pool of locks is used, and each atomic value is associated with the same lock throughout its lifetime, using a hash of its pointer address.  I also reworked how atomics are recorded/replayed to avoid using a mutex and its associated overhead.  This results in a pretty dramatic performance improvement on complex pages.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Specify the address of atomic values when recording ordered atomic accesses.

Change the public record/replay API so that callers specify the address of the atomic value (or a nearby address, as long as the same address is always used for the same atomic) being recorded.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Use new atomic access API in Gecko.

Fix users of the atomic access API outside of toolkit/recordreplay.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Use new atomic access API in internal record/replay logic.

Fix users of the atomic access API within toolkit/recordreplay.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Relax ordering requirements for atomic accesses to be specific to the atomic value itself.

Core logic to use a pool of locks for atomic accesses, and avoid using system mutexes when recording them.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 9020215 [details] [diff] [review]
Part 1 - Specify the address of atomic values when recording ordered atomic accesses.

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

Are you going to collapse these patches in any way so that bisecting continues to work, or are you going to land them as they are?

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Nathan Froyd [:froydnj] from comment #5)
> Comment on attachment 9020215 [details] [diff] [review]
> Part 1 - Specify the address of atomic values when recording ordered atomic
> accesses.
> 
> Review of attachment 9020215 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are you going to collapse these patches in any way so that bisecting
> continues to work, or are you going to land them as they are?

When I've been landing these patch series I've landed the patches as they were reviewed, so people looking at the revision history can match the landed code up with the actual changes in the bug.  I haven't really thought about bisection but shouldn't bisection tools be smart enough to either focus on the push log rather than the change log, or just ignore changesets that don't build or are otherwise broken?

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Brian Hackett (:bhackett) from comment #6)
> (In reply to Nathan Froyd [:froydnj] from comment #5)
> > Are you going to collapse these patches in any way so that bisecting
> > continues to work, or are you going to land them as they are?
> 
> When I've been landing these patch series I've landed the patches as they
> were reviewed, so people looking at the revision history can match the
> landed code up with the actual changes in the bug.  I haven't really thought
> about bisection but shouldn't bisection tools be smart enough to either
> focus on the push log rather than the change log, or just ignore changesets
> that don't build or are otherwise broken?

I am pretty sure that common bisection tools know nothing about the pushlog; my impression is that pushlog is a Mozilla-specific thing?  I don't know what the common solution is to non-building changesets, though.

Anyway, if you've been landing the reviewed things thus far, might as well continue that pattern, rather than squashing.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a3d3ccfdd50
Part 1 - Specify the address of atomic values when recording ordered atomic accesses, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ed95dcc001
Part 2 - Fix  useUse new atomic access API in Gecko, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e911b99c9c08
Part 3 - Use new atomic access API in internal record/replay logic, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b31239e8fbe3
Part 4 - Relax ordering requirements for atomic accesses to be specific to the atomic value itself, r=froydnj.

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9a3d3ccfdd50
https://hg.mozilla.org/mozilla-central/rev/c7ed95dcc001
https://hg.mozilla.org/mozilla-central/rev/e911b99c9c08
https://hg.mozilla.org/mozilla-central/rev/b31239e8fbe3