Merge Sampler and GeckoSampler

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
GeckoSampler is the only subclass of Sampler. Might as well merge them.
(Assignee)

Comment 1

2 years ago
Created attachment 8831110 [details] [diff] [review]
(part 1) - Merge Sampler constructors and destructors

They're defined separately for each platform, but the definitions are almost
identical and can be commoned up.
Attachment #8831110 - Flags: review?(mstange)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8831111 [details] [diff] [review]
(part 2) - Merge Sampler and GeckoSampler

There's no point having them as separate classes. This removes the need for
some virtual functions, too.
Attachment #8831111 - Flags: review?(mstange)
(Assignee)

Comment 3

2 years ago
Created attachment 8831113 [details] [diff] [review]
(part 3) - Rename GeckoSampler.cpp as Sampler.cpp

The patch also moves some Sampler methods from platform.cpp to Sampler.cpp. Now
all Sampler methods are in Sampler.cpp except for a small number of
platform-specific ones, which are in platform-*.cpp.
Attachment #8831113 - Flags: review?(mstange)
(Assignee)

Comment 4

2 years ago
Created attachment 8831114 [details] [diff] [review]
(part 4) - Remove Sampler::profiling_

Because it's always true.
Attachment #8831114 - Flags: review?(mstange)
(Assignee)

Comment 5

2 years ago
Created attachment 8831116 [details] [diff] [review]
(part 5) - Remove PlatformData::profiled_pthread_

It's unused.
Attachment #8831116 - Flags: review?(mstange)
Attachment #8831110 - Flags: review?(mstange) → review+
Attachment #8831111 - Flags: review?(mstange) → review+
Comment on attachment 8831113 [details] [diff] [review]
(part 3) - Rename GeckoSampler.cpp as Sampler.cpp

Review of attachment 8831113 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/core/Sampler.cpp
@@ +165,5 @@
>    return false;
>  }
>  
> +std::vector<ThreadInfo*>* Sampler::sRegisteredThreads = nullptr;
> +mozilla::UniquePtr< ::Mutex> Sampler::sRegisteredThreadsMutex;

Shouldn't this be mozilla::Mutex? Looks like I missed this in the patch that removed ::Mutex.
Attachment #8831113 - Flags: review?(mstange) → review+
Attachment #8831114 - Flags: review?(mstange) → review+
Attachment #8831116 - Flags: review?(mstange) → review+
(Assignee)

Comment 7

2 years ago
> > +std::vector<ThreadInfo*>* Sampler::sRegisteredThreads = nullptr;
> > +mozilla::UniquePtr< ::Mutex> Sampler::sRegisteredThreadsMutex;
> 
> Shouldn't this be mozilla::Mutex? Looks like I missed this in the patch that
> removed ::Mutex.

The declaration in platform.h uses mozilla::Mutex. So ::Mutex must somehow auto-qualify as mozilla::Mutex. But I've changed it locally for clarity.
You need to log in before you can comment on or make changes to this bug.