Merge Sampler.cpp into platform.cpp

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years 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

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

2 years ago
Posted patch (part 1) - Rename sStartTime (obsolete) — Splinter Review
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

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

Updated

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

Comment 4

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

Comment 5

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

Comment 6

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

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

Comment 8

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

Comment 10

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

Updated

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

Comment 11

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

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

Updated

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

Updated

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

Comment 18

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

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

Updated

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

Comment 23

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

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

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

Comment 29

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

Comment 30

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

Comment 31

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

Comment 35

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

Comment 37

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

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

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

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

Updated

2 years ago
Keywords: leave-open
(Assignee)

Updated

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

Updated

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