Closed Bug 1756251 Opened 2 years ago Closed 2 years ago

Document thread safety guarantees of GeckoJavaSampler

Categories

(Core :: Gecko Profiler, task, P2)

task

Tracking

()

RESOLVED FIXED
99 Branch
Tracking Status
firefox99 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(7 files)

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.

This class already appears to be thread safe so I documented the policy I saw.

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

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

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

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

I documented the existing way we maintain thread safety in this class.

Depends on D139197

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

Severity: -- → N/A
Priority: -- → P3

Not just documenting, but making things more thread-safe, it's probably quite important -> P2.
Thank you Michael.

Priority: P3 → P2
Attachment #9264674 - Attachment description: Bug 1756251 - synchronize access to GeckoJavaSampler.isProfilerActive. r=canaltinova → Bug 1756251 - fix concurrency bug in GeckoJavaSampler.isProfilerActive. r=canaltinova
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: