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•2 years ago
|
||
This class already appears to be thread safe so I documented the policy I saw.
Assignee | ||
Comment 2•2 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•2 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•2 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•2 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•2 years ago
|
||
I documented the existing way we maintain thread safety in this class.
Depends on D139197
Assignee | ||
Comment 7•2 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•2 years ago
|
Pushed by mcomella@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f40ca4d69fb7 document thread safety of GeckoJavaSampler.MarkerStorage. r=julienw https://hg.mozilla.org/integration/autoland/rev/c216f871ebea make GeckoJavaSampler.Frame thread safe. r=geckoview-reviewers,julienw,agi https://hg.mozilla.org/integration/autoland/rev/91c8c4fca863 make GeckoJavaSampler.Marker thread safe. r=julienw https://hg.mozilla.org/integration/autoland/rev/173f4cd64079 make GeckoJavaSampler.Sample effectively immutable. r=julienw https://hg.mozilla.org/integration/autoland/rev/7c1d425a7ec7 improve GeckoJavaSampler.SamplingRunnable thread safety. r=julienw https://hg.mozilla.org/integration/autoland/rev/3a3dc9dce6ea document thread safety of GeckoJavaSampler. r=julienw https://hg.mozilla.org/integration/autoland/rev/98314f6b05ee fix concurrency bug in GeckoJavaSampler.isProfilerActive. r=geckoview-reviewers,julienw,agi
Comment 10•2 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
•