Merge Sampler.cpp into platform.cpp

RESOLVED FIXED in Firefox 54

Status

()

Core
Gecko Profiler
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(28 attachments, 4 obsolete attachments)

10.58 KB, patch
mstange
: review+
Details | Diff | Splinter Review
3.27 KB, patch
mstange
: review+
Details | Diff | Splinter Review
2.12 KB, patch
mstange
: review+
Details | Diff | Splinter Review
10.50 KB, patch
mstange
: review+
Details | Diff | Splinter Review
9.67 KB, patch
mstange
: review+
Details | Diff | Splinter Review
1.46 KB, patch
mstange
: review+
Details | Diff | Splinter Review
11.78 KB, patch
mstange
: review+
Details | Diff | Splinter Review
5.73 KB, patch
mstange
: review+
Details | Diff | Splinter Review
5.05 KB, patch
mstange
: review+
Details | Diff | Splinter Review
3.47 KB, patch
mstange
: review+
Details | Diff | Splinter Review
7.05 KB, patch
mstange
: review+
Details | Diff | Splinter Review
7.58 KB, patch
mstange
: review+
Details | Diff | Splinter Review
1.79 KB, patch
mstange
: review+
Details | Diff | Splinter Review
9.87 KB, patch
mstange
: review+
Details | Diff | Splinter Review
16.63 KB, patch
mstange
: review+
Details | Diff | Splinter Review
11.73 KB, patch
mstange
: review+
Details | Diff | Splinter Review
19.33 KB, patch
mstange
: review+
Details | Diff | Splinter Review
3.00 KB, patch
mstange
: review+
Details | Diff | Splinter Review
4.83 KB, patch
mstange
: review+
Details | Diff | Splinter Review
57.88 KB, patch
mstange
: review+
Details | Diff | Splinter Review
8.39 KB, patch
mstange
: review+
Details | Diff | Splinter Review
4.33 KB, patch
mstange
: review+
Details | Diff | Splinter Review
5.40 KB, patch
mstange
: review+
Details | Diff | Splinter Review
5.73 KB, patch
mstange
: review+
Details | Diff | Splinter Review
5.35 KB, patch
mstange
: review+
Details | Diff | Splinter Review
27.11 KB, patch
mstange
: review+
Details | Diff | Splinter Review
11.45 KB, patch
mstange
: review+
Details | Diff | Splinter Review
1.37 KB, patch
schien
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 months ago
Sampler is a class that only ever has zero or one instances, depending on whether the profiler is running. All of its state is effectively global; even when it's not instantiated, profiler_*() functions often need to do something stateful anyway. (See the sIs* variables in platform.cpp for an unpleasant and illustrative example.)

platform.cpp already has a number of global variables which are accessed via the profiler_*() functions. Merging Sampler into platform.cpp will remove an unhelpful layer of separation and put all the relevant code in one file. Sampler's fields will become global variables in platform.cpp.

These changes will make the code simpler, more uniform, and more readable. It will also make it much easier to see which state is accessed from multiple threads and thus needs synchronization.
(Assignee)

Comment 1

6 months ago
Created attachment 8834197 [details] [diff] [review]
(part 1) - Rename sStartTime

This function indictes when profiler_init() is called, not profiler_start().
Renaming it as sInitTime makes that clearer.
Attachment #8834197 - Flags: review?(mstange)
(Assignee)

Comment 2

6 months ago
Created attachment 8834201 [details] [diff] [review]
(part 2) - Add more threadednesss assertions and comments to the profiler

This patch adds NS_IsMainThread() assertions to all main-thread-only
profiler_*() functions that currently lack them, and adds comments to those
that run on multiple threads. As a result, it's now clear for every
profiler_*() function which threads it runs on.
Attachment #8834201 - Flags: review?(mstange)
(Assignee)

Comment 3

6 months ago
Created attachment 8834203 [details] [diff] [review]
(part 2) - Add more threadedness assertions and comments to the profiler
Attachment #8834203 - Flags: review?(mstange)
(Assignee)

Updated

6 months ago
Attachment #8834201 - Attachment is obsolete: true
Attachment #8834201 - Flags: review?(mstange)
(Assignee)

Comment 4

6 months ago
Created attachment 8834210 [details] [diff] [review]
(part 3) - Remove profiler_responsiveness

All it does is set sLastTracerEvent, which is never read.
Attachment #8834210 - Flags: review?(mstange)
(Assignee)

Comment 5

6 months ago
Created attachment 8834211 [details] [diff] [review]
(part 4) - Move the declaration of gSampler

It doesn't need to be visible outside the profiler.
Attachment #8834211 - Flags: review?(mstange)
(Assignee)

Comment 6

6 months ago
Created attachment 8834212 [details] [diff] [review]
(part 5) - #include Sampler.cpp from platform.cpp

I am planning to merge Sampler into platform.cpp, so Sampler.cpp will
disappear. This change will make that easier, because things that temporarily
need to be visible in both files won't need to be declared in a header.
Attachment #8834212 - Flags: review?(mstange)
(Assignee)

Comment 7

6 months ago
Created attachment 8834213 [details] [diff] [review]
(part 6) - Move sRegisteredThreads{,Mutex} out of Sampler

They don't particularly belong there, and Sampler will be going away eventually
anyway.
Attachment #8834213 - Flags: review?(mstange)
(Assignee)

Comment 8

6 months ago
Created attachment 8834215 [details] [diff] [review]
(part 7) - Move Startup(), Shutdown(), RegisterCurrentThread(), UnregisterCurrentThread() out of Sampler

This is a good example of code that's in Sampler for no particular reason.
Attachment #8834215 - Flags: review?(mstange)
(Assignee)

Comment 9

6 months ago
Created attachment 8834220 [details] [diff] [review]
(part 8) - Move mGatherer out of Sampler
Attachment #8834220 - Flags: review?(mstange)
(Assignee)

Comment 10

6 months ago
Created attachment 8834240 [details] [diff] [review]
(part 1) - Add a comment about sStartTime

Whoops, I completely misunderstood how this variable is used in my previous
patch.
Attachment #8834240 - Flags: review?(mstange)
(Assignee)

Updated

6 months ago
Attachment #8834197 - Attachment is obsolete: true
Attachment #8834197 - Flags: review?(mstange)
(Assignee)

Comment 11

6 months ago
Created attachment 8834250 [details] [diff] [review]
(part 9) - Move mThreadNameFilters and mFeatures out of Sampler

Doing so made it clearer that gThreadNameFilters was being accessed from
multiple threads without synchronization, so I added a Mutex for it.
Attachment #8834250 - Flags: review?(mstange)
(Assignee)

Comment 12

6 months ago
Created attachment 8834251 [details] [diff] [review]
(part 10) - Move entrySize_ out of Sampler
Attachment #8834251 - Flags: review?(mstange)
(Assignee)

Comment 13

6 months ago
Created attachment 8834258 [details] [diff] [review]
(part 11) - Move the Linux-specific fields out of Sampler
Attachment #8834258 - Flags: review?(mstange)
(Assignee)

Comment 14

6 months ago
Created attachment 8834260 [details] [diff] [review]
(part 12) - Move s{,Last}FrameNumber from Sampler.cpp to platform.cpp
Attachment #8834260 - Flags: review?(mstange)
(Assignee)

Comment 15

6 months ago
Created attachment 8834268 [details] [diff] [review]
(part 4) - #include platform-*.cc from platform.cpp

platform-*.cc and platform.cpp belong together conceptually, and combining them
into a single compilation unit makes it easier to share things and avoids the
need for some declarations in headers.

The patch also removes old_sigsave_signal_handler_ which is a long-dead field
that clang now detects and complains about.
Attachment #8834268 - Flags: review?(mstange)
(Assignee)

Comment 16

6 months ago
Created attachment 8834271 [details] [diff] [review]
(part 6) - Move sRegisteredThreads{,Mutex} out of Sampler
Attachment #8834271 - Flags: review?(mstange)
(Assignee)

Updated

6 months ago
Attachment #8834213 - Attachment is obsolete: true
Attachment #8834213 - Flags: review?(mstange)
(Assignee)

Comment 17

6 months ago
Created attachment 8834275 [details] [diff] [review]
(part 6b) - Move declarations of gSampler
Attachment #8834275 - Flags: review?(mstange)
(Assignee)

Updated

6 months ago
Attachment #8834211 - Attachment is obsolete: true
Attachment #8834211 - Flags: review?(mstange)
(Assignee)

Comment 18

6 months ago
Apologies for the patch shuffling; while writing later patches I realized I could improve some of the earlier ones.

I have more to do here, but these patches give you a good idea of the direction I'm heading in.
Attachment #8834240 - Flags: review?(mstange) → review+
Comment on attachment 8834203 [details] [diff] [review]
(part 2) - Add more threadedness assertions and comments to the profiler

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

::: tools/profiler/core/platform.cpp
@@ +1078,5 @@
>  profiler_time(const mozilla::TimeStamp& aTime)
>  {
> +  // This function currently only used on the main thread, but that restriction
> +  // (and this assertion) could be removed trivially because it doesn't touch
> +  // data that requires locking.

It touches sStartTime, so I don't think this comment is correct.
Attachment #8834203 - Flags: review?(mstange) → review+
Attachment #8834210 - Flags: review?(mstange) → review+
Attachment #8834268 - Flags: review?(mstange) → review+
Attachment #8834212 - Flags: review?(mstange) → review+
Attachment #8834215 - Flags: review?(mstange) → review+
Attachment #8834220 - Flags: review?(mstange) → review+
Attachment #8834250 - Flags: review?(mstange) → review+
Attachment #8834251 - Flags: review?(mstange) → review+
Attachment #8834258 - Flags: review?(mstange) → review+
Attachment #8834260 - Flags: review?(mstange) → review+
Attachment #8834271 - Flags: review?(mstange) → review+
Attachment #8834275 - Flags: review?(mstange) → review+
Do you have plans to reduce the number of global variables and locks once this is done and the access + usage patterns have become clearer? For example we should be able to group some of these fields into structs, and then maybe use a single mutex for all accesses of the same group / struct. For example the init params (set by profiler_start, read by any thread) seem like they should be such a group.
(Assignee)

Comment 21

6 months ago
(In reply to Markus Stange [:mstange] from comment #20)
> Do you have plans to reduce the number of global variables and locks once
> this is done and the access + usage patterns have become clearer?

Yes! Not sure yet what it will look like, but I definitely want to do something like that.
(Assignee)

Comment 22

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2aa39f93cbcf6b69f4ee2dafc505010850daa87
Bug 1337189 (part 1) - Add a comment about sStartTime. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/499daf10f2ff638df8400e56cbd217d61a47be8c
Bug 1337189 (part 2) - Add more threadedness assertions and comments to the profiler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f55b285bad3f6f84035bb392fde332f76fd6f4c
Bug 1337189 (part 3) - Remove profiler_responsiveness. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6f643603520c375fdcea9e8f0273d0862e7b783
Bug 1337189 (part 4) - #include platform-*.cc from platform.cpp. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb286937e67a44ae1de84157dabe1f50b0d2562
Bug 1337189 (part 5) - #include Sampler.cpp from platform.cpp. r=mstange.
(Assignee)

Updated

6 months ago
Keywords: leave-open
Summary: Merge Sampler into profiler.cpp → Merge Sampler into platform.cpp
(Assignee)

Comment 23

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/85905c454c0553c12ef3db1f12415229b9c980c5
Bug 1337189 (part 6) - Move sRegisteredThreads{,Mutex} out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/81c0116b6037778653111d4052410722ed434355
Bug 1337189 (part 6b) - Move declarations of gSampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbe13875035b36735dcac2489abfcaa95dc9a69
Bug 1337189 (part 7) - Move Startup(), Shutdown(), RegisterCurrentThread(), UnregisterCurrentThread() out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/572e9915387cd18c0f5198dfbb74b435a550d1ee
Bug 1337189 (part 8) - Move mGatherer out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4920597f7463dca5ae5c0e4eea050ceb9d9c716a
Bug 1337189 (part 9) - Move mThreadNameFilters and mFeatures out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/df5a1de42f04e5d4e5e551d28d049911a4035f2d
Bug 1337189 (part 10) - Move entrySize_ out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b47c959f0c6a49786cecf930f869436939f0454
Bug 1337189 (part 11) - Move the Linux-specific fields out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/46234bd1f1d778eb2c58b859673644a7890f0ac7
Bug 1337189 (part 12) - Move s{,Last}FrameNumber from Sampler.cpp to platform.cpp. r=mstange.
(Assignee)

Comment 24

6 months ago
Created attachment 8835336 [details] [diff] [review]
(part 13) - Move interval_ out of Sampler
Attachment #8835336 - Flags: review?(mstange)
(Assignee)

Comment 25

6 months ago
Created attachment 8835337 [details] [diff] [review]
(part 14) - Move active_ and paused_ out of Sampler
Attachment #8835337 - Flags: review?(mstange)
(Assignee)

Comment 26

6 months ago
Created attachment 8835338 [details] [diff] [review]
(part 15) - Move mBuffer out of Sampler
Attachment #8835338 - Flags: review?(mstange)
(Assignee)

Comment 27

6 months ago
Created attachment 8835339 [details] [diff] [review]
(part 16) - Move all the bools out of Sampler

Two of them (mAddMainThreadIO and mPrivacyMode) are now local variables within
profiler_start(), which is nice.

This change also lets us get rid of four sIs* variables.
Attachment #8835339 - Flags: review?(mstange)
(Assignee)

Comment 28

6 months ago
Created attachment 8835340 [details] [diff] [review]
(part 17) - Remove Sampler::FlushOnJSShutdown()

This means PseudoStack::flushSamplerOnJSShutdown() should now work when called
off-main-thread.
Attachment #8835340 - Flags: review?(mstange)
(Assignee)

Comment 29

6 months ago
Created attachment 8835341 [details] [diff] [review]
(part 18) - Remove Sampler::GetBacktrace()

The patch also removes NewSyncProfile() by inlining it at its single call site.
Attachment #8835341 - Flags: review?(mstange)
(Assignee)

Comment 30

6 months ago
Created attachment 8835342 [details] [diff] [review]
(part 19) - Move Tick() and GetThreadHandle() out of sampler

The patch also removes InplaceTick(), which wasn't serving any useful purpose.
Attachment #8835342 - Flags: review?(mstange)
(Assignee)

Comment 31

6 months ago
Created attachment 8835344 [details] [diff] [review]
(part 20) - Move Start() and Stop() out of Sampler

The patch also renames them as PlatformStart() and PlatformStop() to make it
clearer they are platform-specific.
Attachment #8835344 - Flags: review?(mstange)
(Assignee)

Comment 32

6 months ago
Created attachment 8835345 [details] [diff] [review]
(part 21) - Remove Sampler::SizeOfIncludingThis()
Attachment #8835345 - Flags: review?(mstange)
(Assignee)

Comment 33

6 months ago
Created attachment 8835346 [details] [diff] [review]
(part 22) - Move CanNotifyObservers() out of Sampler
Attachment #8835346 - Flags: review?(mstange)
(Assignee)

Comment 34

6 months ago
Created attachment 8835347 [details] [diff] [review]
(part 23) - Move PlatformData out of Sampler
Attachment #8835347 - Flags: review?(mstange)
(Assignee)

Comment 35

6 months ago
Created attachment 8835348 [details] [diff] [review]
(part 24) - Move RegisterThread() out of Sampler and rename it

The new name is MaybeSetProfile(), which better reflects what it does.
Attachment #8835348 - Flags: review?(mstange)
(Assignee)

Comment 36

6 months ago
Created attachment 8835349 [details] [diff] [review]
(part 25) - Move streaming/saving code out of Sampler
Attachment #8835349 - Flags: review?(mstange)
(Assignee)

Comment 37

6 months ago
Created attachment 8835350 [details] [diff] [review]
(part 26) - Remove Sampler.cpp

Sampler's constructor and destructor are inlined into profiler_{start,stop}(),
respectively. A trivial Sampler class is left behind (now in platform.cpp)
because there are still numerous gSampler null checks, which I will remove in a
follow-up bug.

PseudoStack::flushSamplerOnShutdown() moves into platform.cpp. MAXPATHLEN is
unused and is deleted.
Attachment #8835350 - Flags: review?(mstange)
(Assignee)

Comment 38

6 months ago
After those 26 patches Sampler.cpp is gone but the Sampler class still exists, albeit trivially. That is enough patches for this bug, I'll remove Sampler and do other improvements in follow-up bugs.
Summary: Merge Sampler into platform.cpp → Merge Sampler.cpp into platform.cpp

Comment 39

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b2aa39f93cbc
https://hg.mozilla.org/mozilla-central/rev/499daf10f2ff
https://hg.mozilla.org/mozilla-central/rev/6f55b285bad3
https://hg.mozilla.org/mozilla-central/rev/e6f643603520
https://hg.mozilla.org/mozilla-central/rev/9fb286937e67
https://hg.mozilla.org/mozilla-central/rev/85905c454c05
https://hg.mozilla.org/mozilla-central/rev/81c0116b6037
https://hg.mozilla.org/mozilla-central/rev/3cbe13875035
https://hg.mozilla.org/mozilla-central/rev/572e9915387c
https://hg.mozilla.org/mozilla-central/rev/4920597f7463
https://hg.mozilla.org/mozilla-central/rev/df5a1de42f04
https://hg.mozilla.org/mozilla-central/rev/5b47c959f0c6
https://hg.mozilla.org/mozilla-central/rev/46234bd1f1d7
Attachment #8835336 - Flags: review?(mstange) → review+
Attachment #8835337 - Flags: review?(mstange) → review+
Attachment #8835338 - Flags: review?(mstange) → review+
Attachment #8835339 - Flags: review?(mstange) → review+
Attachment #8835340 - Flags: review?(mstange) → review+
Attachment #8835341 - Flags: review?(mstange) → review+
Comment on attachment 8835342 [details] [diff] [review]
(part 19) - Move Tick() and GetThreadHandle() out of sampler

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

::: tools/profiler/core/platform.cpp
@@ +564,5 @@
> +  StackWalkCallback(/* frameNum */ 0, aSample->pc, aSample->sp, &nativeStack);
> +
> +  uint32_t maxFrames = uint32_t(nativeStack.size - nativeStack.count);
> +
> +  // Win X64 doesn't support disabling frame pointers emission so we need to

Might want to fix that comment up to say "frame pointer omission" while you're here.
Attachment #8835342 - Flags: review?(mstange) → review+
Comment on attachment 8835344 [details] [diff] [review]
(part 20) - Move Start() and Stop() out of Sampler

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

::: tools/profiler/core/platform.cpp
@@ +1535,5 @@
>  }
>  
> +// Platform-specific start/stop actions.
> +static void PlatformStart();
> +static void PlatformStop();

Is it intentional that the declaration has "static" but the definitions don't?
Attachment #8835344 - Flags: review?(mstange) → review+
Comment on attachment 8835345 [details] [diff] [review]
(part 21) - Remove Sampler::SizeOfIncludingThis()

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

::: tools/profiler/core/platform.cpp
@@ +1149,5 @@
> +  // Measurement of the following things may be added later if DMD finds it
> +  // is worthwhile:
> +  // - memory pointed to by the elements within gBuffer
> +  // - mThreadNameFilters
> +  // - mFeatures

These are called gThreadNameFilters and gFeatures now.
Attachment #8835345 - Flags: review?(mstange) → review+
Comment on attachment 8835346 [details] [diff] [review]
(part 22) - Move CanNotifyObservers() out of Sampler

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

::: tools/profiler/core/platform.cpp
@@ +199,5 @@
> +
> +#ifdef MOZ_WIDGET_GONK
> +  // We use profile.sh on b2g to manually select threads and options per
> +  // process.
> +  return false;

Oh, this stuff can go for sure. We're  not using profile.sh anymore.
Attachment #8835346 - Flags: review?(mstange) → review+
Attachment #8835347 - Flags: review?(mstange) → review+
Attachment #8835348 - Flags: review?(mstange) → review+
Attachment #8835349 - Flags: review?(mstange) → review+
Attachment #8835350 - Flags: review?(mstange) → review+
(Assignee)

Comment 44

6 months ago
> > +  // Win X64 doesn't support disabling frame pointers emission so we need to
> 
> Might want to fix that comment up to say "frame pointer omission" while
> you're here.

LOL


> > +// Platform-specific start/stop actions.
> > +static void PlatformStart();
> > +static void PlatformStop();
> 
> Is it intentional that the declaration has "static" but the definitions
> don't?

Not particularly. I will add "static" to the definitions.
(Assignee)

Comment 45

6 months ago
(In reply to Nicholas Nethercote [:njn] from comment #44)
> > > +  // Win X64 doesn't support disabling frame pointers emission so we need to
> > 
> > Might want to fix that comment up to say "frame pointer omission" while
> > you're here.
> 
> LOL

Actually, that comment is awful -- the triple negative makes it hard to read. Here's my revised version:

  // Win64 always omits frame pointers so for it we fall back to using the
  // slower MozStackWalk().
Ok, that sounds much better, thanks.
(Assignee)

Comment 47

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc0ad9814a6c6475c6a2b30cc6296e3a73730a4e
Bug 1337189 (part 13) - Move interval_ out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8694332bf02aac93abb471b334e0b51721cd3628
Bug 1337189 (part 14) - Move active_ and paused_ out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9c499755393bb02be0c0f264bfe8ec3addf9fd
Bug 1337189 (part 15) - Move mBuffer out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a894b2316f0a6910aeb6243060497ef277ed847
Bug 1337189 (part 16) - Move all the bools out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/99c10f2d0d76fe91dacb340cf3b3ef0ac50976b5
Bug 1337189 (part 17) - Remove Sampler::FlushOnJSShutdown(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a94811a6ef0d9eff21ae877e28cbf9d96bf1e31
Bug 1337189 (part 18) - Remove Sampler::GetBacktrace(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/778675533fb9c6ab7f33a32c94fd20baf5436db9
Bug 1337189 (part 19) - Move Tick() and GetThreadHandle() out of sampler. r=mstange.
(Assignee)

Comment 48

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c677b0b92faa62a6694b4fb7d44f44371923b2
Bug 1337189 (part 20) - Move Start() and Stop() out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/63172ee12b036c5201d61a48bc242767e1e069b3
Bug 1337189 (part 21) - Remove Sampler::SizeOfIncludingThis(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c209f8f314b33337c9fee50bcb7bb87236fab6b
Bug 1337189 (part 22) - Move CanNotifyObservers() out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b603e66e2765ea37409b72595bf16f27c3d91a9
Bug 1337189 (part 23) - Move PlatformData out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5805116bb2e09ba42b7c385fc867e3ac31575730
Bug 1337189 (part 24) - Move RegisterThread() out of Sampler and rename it. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa0f4dd3ce4742efc6fe68d528f3ab13fa6e9dcc
Bug 1337189 (part 25) - Move streaming/saving code out of Sampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1f662d5bc132611069fe0bbfa5c77ea8be1ed4
Bug 1337189 (part 26) - Remove Sampler.cpp. r=mstange.
(Assignee)

Updated

6 months ago
Keywords: leave-open

Comment 49

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc0ad9814a6c
https://hg.mozilla.org/mozilla-central/rev/8694332bf02a
https://hg.mozilla.org/mozilla-central/rev/9d9c49975539
https://hg.mozilla.org/mozilla-central/rev/4a894b2316f0
https://hg.mozilla.org/mozilla-central/rev/99c10f2d0d76
https://hg.mozilla.org/mozilla-central/rev/6a94811a6ef0
https://hg.mozilla.org/mozilla-central/rev/778675533fb9
https://hg.mozilla.org/mozilla-central/rev/d4c677b0b92f
https://hg.mozilla.org/mozilla-central/rev/63172ee12b03
https://hg.mozilla.org/mozilla-central/rev/6c209f8f314b
https://hg.mozilla.org/mozilla-central/rev/3b603e66e276
https://hg.mozilla.org/mozilla-central/rev/5805116bb2e0
https://hg.mozilla.org/mozilla-central/rev/aa0f4dd3ce47
https://hg.mozilla.org/mozilla-central/rev/8c1f662d5bc1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

6 months ago
Blocks: 1328363
Comment on attachment 8835339 [details] [diff] [review]
(part 16) - Move all the bools out of Sampler

This patch causes build error while enabling TaskTracer.
>15:00.54 In file included from mozilla-central/obj-firefox.noindex/tools/profiler/Unified_cpp_tools_profiler0.cpp:74:
>15:00.54 mozilla-central/tools/profiler/core/platform.cpp:2053:7: error: use of undeclared identifier 'mTaskTracer'; did you mean 'gTaskTracer'?
>15:00.54   if (mTaskTracer) {
>15:00.54       ^~~~~~~~~~~
>15:00.54       gTaskTracer
>15:00.54 mozilla-central/tools/profiler/core/platform.cpp:163:13: note: 'gTaskTracer' declared here
>15:00.54 static bool gTaskTracer = false;
>15:00.54             ^
>15:00.54 mozilla-central/tools/profiler/core/platform.cpp:2214:7: error: use of undeclared identifier 'mTaskTracer'; did you mean 'gTaskTracer'?
>15:00.54   if (mTaskTracer) {
>15:00.54       ^~~~~~~~~~~
>15:00.54       gTaskTracer
>15:00.54 mozilla-central/tools/profiler/core/platform.cpp:163:13: note: 'gTaskTracer' declared here
>15:00.54 static bool gTaskTracer = false;
>15:00.54             ^
>15:00.54 2 errors generated.
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 51

6 months ago
Created attachment 8836582 [details] [diff] [review]
(follow-up) - Fix MOZ_TASK_TRACER bustage
Attachment #8836582 - Flags: review?(schien)
Comment on attachment 8836582 [details] [diff] [review]
(follow-up) - Fix MOZ_TASK_TRACER bustage

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

Thanks!
Attachment #8836582 - Flags: review?(schien) → review+
(Assignee)

Comment 53

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/96caedfc0c821b181ba811f3a550ca9ed7e06434
Bug 1337189 (follow-up) - Fix MOZ_TASK_TRACER bustage. r=schien.

Comment 54

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/96caedfc0c82
(Assignee)

Updated

6 months ago
Flags: needinfo?(n.nethercote)
Depends on: 1340327
You need to log in before you can comment on or make changes to this bug.