Closed Bug 1337189 Opened 4 years ago Closed 4 years ago

Merge Sampler.cpp into platform.cpp

Categories

(Core :: Gecko Profiler, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(28 files, 4 obsolete files)

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
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.
Attached 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)
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)
Attachment #8834201 - Attachment is obsolete: true
Attachment #8834201 - Flags: review?(mstange)
All it does is set sLastTracerEvent, which is never read.
Attachment #8834210 - Flags: review?(mstange)
It doesn't need to be visible outside the profiler.
Attachment #8834211 - Flags: review?(mstange)
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)
They don't particularly belong there, and Sampler will be going away eventually
anyway.
Attachment #8834213 - Flags: review?(mstange)
This is a good example of code that's in Sampler for no particular reason.
Attachment #8834215 - Flags: review?(mstange)
Whoops, I completely misunderstood how this variable is used in my previous
patch.
Attachment #8834240 - Flags: review?(mstange)
Attachment #8834197 - Attachment is obsolete: true
Attachment #8834197 - Flags: review?(mstange)
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)
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)
Attachment #8834213 - Attachment is obsolete: true
Attachment #8834213 - Flags: review?(mstange)
Attachment #8834211 - Attachment is obsolete: true
Attachment #8834211 - Flags: review?(mstange)
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.
(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.
Keywords: leave-open
Summary: Merge Sampler into profiler.cpp → Merge Sampler into platform.cpp
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.
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)
This means PseudoStack::flushSamplerOnJSShutdown() should now work when called
off-main-thread.
Attachment #8835340 - Flags: review?(mstange)
The patch also removes NewSyncProfile() by inlining it at its single call site.
Attachment #8835341 - Flags: review?(mstange)
The patch also removes InplaceTick(), which wasn't serving any useful purpose.
Attachment #8835342 - Flags: review?(mstange)
The patch also renames them as PlatformStart() and PlatformStop() to make it
clearer they are platform-specific.
Attachment #8835344 - Flags: review?(mstange)
The new name is MaybeSetProfile(), which better reflects what it does.
Attachment #8835348 - Flags: review?(mstange)
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)
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+
> > +  // 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.
(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.
Keywords: leave-open
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+
Flags: needinfo?(n.nethercote)
Depends on: 1340327
You need to log in before you can comment on or make changes to this bug.