Closed Bug 1328378 Opened 7 years ago Closed 7 years ago

Clean up SyncProfile + ThreadProfile + ProfileBuffer interaction

Categories

(Core :: Gecko Profiler, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox53 --- affected

People

(Reporter: mstange, Assigned: n.nethercote)

References

Details

Attachments

(6 files)

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.
Also,

> ThreadProfile suggests that it owns its own ProfileBuffer, but it really
> aliases the global TableTicker's ProfileBuffer.
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
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)
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)
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)
It's silly indirection.
Attachment #8841394 - Flags: review?(mstange)
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)
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)
Attachment #8841391 - Flags: review?(mstange) → review+
Attachment #8841392 - Flags: review?(mstange) → review+
Attachment #8841393 - Flags: review?(mstange) → review+
Attachment #8841394 - Flags: review?(mstange) → review+
Keywords: leave-open
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+
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+
> 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.
(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.
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.