Document thread safety guarantees of GeckoJavaSampler
Categories
(Core :: Gecko Profiler, task, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox99 | --- | fixed |
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(7 files)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review |
The GeckoJavaSampler class has synchronized blocks throughout the code indicating it's intended to be accessed from multiple threads. However, there is no documentation describing what the existing thread safety policies are, making it difficult to add to the class. Let's document those.
|
Assignee | |
Comment 1•4 years ago
|
||
This class already appears to be thread safe so I documented the policy I saw.
|
|
Assignee | |
Comment 2•4 years ago
|
||
This class was not thread safe because the fields in this class were mutable
and access to mutable state must be synchronized across threads. This class was
only used in an immutable fashion, however, so it was trivial to make it
immutable and thus thread safe.
Depends on D139193
|
|
Assignee | |
Comment 3•4 years ago
|
||
Like Frame, this class was not thread safe because it exposed mutable state.
However, it was being used in an immutable manner so it was trivial to make it
thread safe.
Depends on D139194
|
|
Assignee | |
Comment 4•4 years ago
|
||
As used, the class is effectively immutable which provides some thread safety
guarantees so I updated the declarations so the class could take advantage of
that.
Effectively immutable classes need to be safely published in order to be thread
safe. I believe we could avoid the need for safe publishing (i.e. strengthen
thread safety guarantees) if we replaced the Frame[] type with a
thread safe data type (AtomicReferenceArray, a concurrent list, etc.).
Depends on D139195
|
|
Assignee | |
Comment 5•4 years ago
|
||
getSample's callers are synchronized in the current implementation so there are
no concurrency bugs there but it can later be called by an unsynchronized caller
so I added synchronization to it and documented the thread safety guarantees.
Depends on D139196
|
|
Assignee | |
Comment 6•4 years ago
|
||
I documented the existing way we maintain thread safety in this class.
Depends on D139197
|
|
Assignee | |
Comment 7•4 years ago
|
||
Concurrency bug: this method accesses mutable state but is not synchronized so
the caller isn't guaranteed to see the latest values.
We can fix it by synchronizing access to it. I am mildly concerned that this may
introduce performance issues due to increased lock contention but we can address
that if it's a real problem.
This is the only concurrency bug I found throughout my thread safety
documentation.
Depends on D139198
Not just documenting, but making things more thread-safe, it's probably quite important -> P2.
Thank you Michael.
|
||
Updated•4 years ago
|
|
||
Comment 10•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/f40ca4d69fb7
https://hg.mozilla.org/mozilla-central/rev/c216f871ebea
https://hg.mozilla.org/mozilla-central/rev/91c8c4fca863
https://hg.mozilla.org/mozilla-central/rev/173f4cd64079
https://hg.mozilla.org/mozilla-central/rev/7c1d425a7ec7
https://hg.mozilla.org/mozilla-central/rev/3a3dc9dce6ea
https://hg.mozilla.org/mozilla-central/rev/98314f6b05ee
Description
•