Clean up SyncProfile + ThreadProfile + ProfileBuffer interaction

RESOLVED FIXED

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mstange, Assigned: njn)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected)

Details

Attachments

(6 attachments)

(Reporter)

Description

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

2 years ago
Also,

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

Comment 2

2 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

2 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

2 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

2 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

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

Comment 7

2 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

2 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

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

Updated

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

Updated

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

Updated

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

Updated

2 years ago
Keywords: leave-open
(Reporter)

Comment 11

2 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

2 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

2 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.
(Reporter)

Comment 16

2 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

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