Clean up SyncProfile + ThreadProfile + ProfileBuffer interaction

RESOLVED FIXED

Status

()

Core
Gecko Profiler
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: mstange, Assigned: njn)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected)

Details

Attachments

(6 attachments)

(Reporter)

Description

4 months ago
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

4 months ago
Also,

> ThreadProfile suggests that it owns its own ProfileBuffer, but it really
> aliases the global TableTicker's ProfileBuffer.
(Assignee)

Comment 2

2 months 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

2 months ago
Created attachment 8841391 [details] [diff] [review]
(part 1) - Remove SyncProfile::mOwnerState

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

2 months ago
Created attachment 8841392 [details] [diff] [review]
(part 2) - Remove BeginUnwind() and EndUnwind()

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

2 months ago
Created attachment 8841393 [details] [diff] [review]
(part 3) - Remove unnecessary locking in profiler_get_backtrace()

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

2 months ago
Created attachment 8841394 [details] [diff] [review]
(part 4) - Remove sample_obj

It's silly indirection.
Attachment #8841394 - Flags: review?(mstange)
(Assignee)

Comment 7

2 months ago
Created attachment 8841395 [details] [diff] [review]
(part 5) - Simplify ProfileBuffer handling

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

2 months ago
Created attachment 8841396 [details] [diff] [review]
(part 6) - Eliminate SyncProfile

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

2 months ago
Attachment #8841391 - Flags: review?(mstange) → review+
(Reporter)

Updated

2 months ago
Attachment #8841392 - Flags: review?(mstange) → review+
(Reporter)

Updated

2 months ago
Attachment #8841393 - Flags: review?(mstange) → review+
(Reporter)

Updated

2 months ago
Attachment #8841394 - Flags: review?(mstange) → review+
(Assignee)

Comment 9

2 months 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

2 months ago
Keywords: leave-open

Comment 10

2 months 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

2 months 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

2 months 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

2 months 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

2 months 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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6de72f3b7f8a
https://hg.mozilla.org/mozilla-central/rev/7becdd5d601a
(Reporter)

Comment 16

2 months 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

a month ago
Status: NEW → RESOLVED
Last Resolved: a month ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.