1334466 - Merge Sampler and GeckoSampler

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Nicholas Nethercote [inactive]]

GeckoSampler is the only subclass of Sampler. Might as well merge them.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Merge Sampler and GeckoSampler

There's no point having them as separate classes. This removes the need for
some virtual functions, too.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove Sampler::profiling_

Because it's always true.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove PlatformData::profiled_pthread_

It's unused.

Comment 6

[Markus Stange [:mstange]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

> > +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.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89526e05e79d13c8fa95f2a0d441ce240c52f566
Bug 1334466 (part 1) - Merge Sampler constructors and destructors. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/63af77eff001e461b633a573e563722b2cf45780
Bug 1334466 (part 2) - Merge Sampler and GeckoSampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/66ffe7a006d1e1355176eb6b6f44ffffe2bb4b05
Bug 1334466 (part 3) - Rename GeckoSampler.cpp as Sampler.cpp. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb29b76cb9d5e75343166925ab6b353fc1ec0215
Bug 1334466 (part 4) - Remove Sampler::profiling_. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/864f918423a3d99dfff536de4780f08e4e922364
Bug 1334466 (part 5) - Remove PlatformData::profiled_pthread_. r=mstange.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89526e05e79d
https://hg.mozilla.org/mozilla-central/rev/63af77eff001
https://hg.mozilla.org/mozilla-central/rev/66ffe7a006d1
https://hg.mozilla.org/mozilla-central/rev/cb29b76cb9d5
https://hg.mozilla.org/mozilla-central/rev/864f918423a3