Closed
Bug 1328378
Opened 7 years ago
Closed 7 years ago
Clean up SyncProfile + ThreadProfile + ProfileBuffer interaction
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox53 | --- | affected |
People
(Reporter: mstange, Assigned: n.nethercote)
References
Details
Attachments
(6 files)
4.13 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
3.89 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
1.10 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
6.34 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
44.76 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
9.68 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
From bug 1135236 comment 15: > SyncProfile is a subclass of ThreadProfile. This is because it gets > wrapped by a TickSample and passed to TableTicker::Tick to fill. > TableTicker::Tick expects TickSample to have a pointer to a ThreadProfile > (which contains the ProfileBuffer it fills), so that's why SyncProfile > extends ThreadProfile. > FIX THIS. > > New Architecture Structure? > ThreadProfile no longer contains its own ProfileBuffer mBuffer > TickSample no longer contains a pointer to ThreadProfile *profile > New abstract base type TickBuffer gets defined. Used for both synchronous > and asynchronous sampling. > TickBuffer gets implementations BacktraceBuffer and SamplerBuffer. > SamplerBuffer is circular, BacktraceBuffer grows on demand.
Reporter | ||
Comment 1•7 years ago
|
||
Also,
> ThreadProfile suggests that it owns its own ProfileBuffer, but it really
> aliases the global TableTicker's ProfileBuffer.
Assignee | ||
Comment 2•7 years ago
|
||
I will do this, but I won't do it as suggested in comment 0. Turns out that, with a bit of restructuring, the extra stuff that SyncProfile adds to ThreadInfo is unnecessary. So SyncProfile can be removed. This change then helps a lot with the synchronization of all the global state in bug 1342306.
Assignee: nobody → n.nethercote
Blocks: 1342306
Assignee | ||
Comment 3•7 years ago
|
||
It appears to be a remnant of a time when SyncProfile lifetimes were more complex. Nowadays they are simple. - profiler_get_backtrace() constructs a SyncProfile called |profile|. |profile|'s mOwnerState is REFERENCED. - profiler_get_backtrace() then calls BeginUnwind() and EndUnwind() on |profile|. After the EndUnwind(), |profile->mOwnerState| is always OWNED. - |profile| then is put into the returned ProfilerBacktrace. That ProfilerBacktrace will destroy |profile| in its destructor because ShouldDestroy() always returns true because mOwnerState is always OWNED. The OWNER_DESTROYING and ORPHANED states are never used, and the whole OwnerState type isn't necessary. This patch removes it and ShouldDestroy().
Attachment #8841391 -
Flags: review?(mstange)
Assignee | ||
Comment 4•7 years ago
|
||
Both of these functions are now trivial and identical in both ThreadInfo and SyncProfile. This patch inlines and removes them.
Attachment #8841392 -
Flags: review?(mstange)
Assignee | ||
Comment 5•7 years ago
|
||
There's no need to lock when calling Tick() on a local TickSample that uses a fresh SyncProfile with its own fresh ProfileBuffer -- none of that data can be touched by another thread.
Attachment #8841393 -
Flags: review?(mstange)
Assignee | ||
Comment 6•7 years ago
|
||
It's silly indirection.
Attachment #8841394 -
Flags: review?(mstange)
Assignee | ||
Comment 7•7 years ago
|
||
Currently ThreadInfo objects all share gBuffer, while SyncProfile objects each get their own ProfileBuffer. This patch removes ThreadInfo::mBuffer to reduce this difference, taking us a step towards eliminating SyncProfile. To support this, the patch: - passes in a buffer as an additional argument in a bunch of places where the buffer used to be obtained from a ThreadInfo; - adds an mBuffer field to ProfilerBacktrace; - changes ThreadInfo::SetProfile() to SetHasProfile(); - removes ThreadInfo::{addTag,StoredMarker,bufferGeneration}(), all of which just redirected to ThreadInfo anyway; - changes ProfileBuffer so it's no longer refcounted, which is unnecessary now that gBuffer and ProfilerBacktrace::mBuffer don't have multiple references, which makes their lifetimes obvious. The patch also removes some ThreadInfo& args in functions in platform.cpp, in places where that ThreadInfo is available within the accompanying TickSampler* arg.
Attachment #8841395 -
Flags: review?(mstange)
Assignee | ||
Comment 8•7 years ago
|
||
It's now a very thin wrapper around ThreadInfo, and so can be removed. The patch also has the bonus of setting mIsMainThread correctly for the ThreadInfos that used to be SyncProfiles (i.e. the ones created in profiler_get_backtrace()). As far as I can tell this has only one very minor effect, because that field is only used for those objects to determine how ThreadResponsiveness::Update() dispatches its runnables.
Attachment #8841396 -
Flags: review?(mstange)
Reporter | ||
Updated•7 years ago
|
Attachment #8841391 -
Flags: review?(mstange) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8841392 -
Flags: review?(mstange) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8841393 -
Flags: review?(mstange) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8841394 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/63fa61a6e3d12a7d562e5108706b8d0a75dc7e4b Bug 1328378 (part 1) - Remove SyncProfile::mOwnerState. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/16d6cb0afff570c36c5aea1e4b9f59a86abc75a5 Bug 1328378 (part 2) - Remove BeginUnwind() and EndUnwind(). r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/6b7e1d039956e207ca24ebfc2629ebd3b6f3c3f0 Bug 1328378 (part 3) - Remove unnecessary locking in profiler_get_backtrace(). r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2a029b41ea26131a0abae7d6e304567ff2615a Bug 1328378 (part 4) - Remove sample_obj. r=mstange.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/63fa61a6e3d1 https://hg.mozilla.org/mozilla-central/rev/16d6cb0afff5 https://hg.mozilla.org/mozilla-central/rev/6b7e1d039956 https://hg.mozilla.org/mozilla-central/rev/6c2a029b41ea
Reporter | ||
Comment 11•7 years ago
|
||
Comment on attachment 8841395 [details] [diff] [review] (part 5) - Simplify ProfileBuffer handling Review of attachment 8841395 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/core/platform.cpp @@ +106,5 @@ > > // All accesses to gGatherer are on the main thread, so no locking is needed. > static StaticRefPtr<mozilla::ProfileGatherer> gGatherer; > > +// Always used in conjunction with gThreads. There is no gThreads, AFAICT - do you mean gRegisteredThreads?
Attachment #8841395 -
Flags: review?(mstange) → review+
Reporter | ||
Comment 12•7 years ago
|
||
Comment on attachment 8841396 [details] [diff] [review] (part 6) - Eliminate SyncProfile Review of attachment 8841396 [details] [diff] [review]: ----------------------------------------------------------------- It would be great to make more use of UniquePtr here at some point.
Attachment #8841396 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 13•7 years ago
|
||
> It would be great to make more use of UniquePtr here at some point.
Yes! I briefly tried that but we pass around |ProfileBuffer*| a lot, and my understanding is that these parameters would change to |const UniquePtr&| (because they are borrows) and I didn't want to complicate this patch with that.
Assignee | ||
Comment 14•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6de72f3b7f8abb27024a34faac7fbde24ee6ec8f Bug 1328378 (part 5) - Simplify ProfileBuffer handling. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/7becdd5d601a803789f817c7ae7d41b6f2d2c0d2 Bug 1328378 (part 6) - Eliminate SyncProfile. r=mstange.
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6de72f3b7f8a https://hg.mozilla.org/mozilla-central/rev/7becdd5d601a
Reporter | ||
Comment 16•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #13) > > It would be great to make more use of UniquePtr here at some point. > > Yes! I briefly tried that but we pass around |ProfileBuffer*| a lot, and my > understanding is that these parameters would change to |const UniquePtr&| They could also change to ProfileBuffer& (and get passed *mBuffer.get()), which admittedly is a bit ugly but expresses all the right intentions.
Assignee | ||
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•