Closed Bug 1358074 Opened 3 years ago Closed 3 years ago

Split ProfilerState in two

Categories

(Core :: Gecko Profiler, enhancement)

55 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox55 --- affected

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(7 files)

ProfilerState contains state that is always valid, and some state that is only valid when the profiler is active. I want to split ProfilerState in two, accordingly.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
gPS is declared in GeckoProfiler.h so that it can be tested as non-null in a
couple of functions.

- These checks are of little value, so this patch removes them.

- That lets us remove the ProfilerState and gPS declarations from
  GeckoProfiler.h.

- And, because ProfilerState is now only used within platform.cpp, that lets us
  rename it as PS, which is how we currently refer to it (via a typedef) within
  platform.cpp anyway.
Attachment #8859957 - Flags: review?(mstange)
We measure it in profiler_init(); there's not point overwriting it with the
same value every time profiler_start() is called!

The patch also renames mStartTime as mProcessStartTime to make things clearer.
Attachment #8859958 - Flags: review?(mstange)
A subsequent patch is going to split PS into two classes, and both classes will
need access to these types. So this patch moves them to the top level.
Attachment #8859959 - Flags: review?(mstange)
The new name is more concise and matches common usage elsewhere (e.g.
profiler_start() arguments).
Attachment #8859960 - Flags: review?(mstange)
It's not needed.
Attachment #8859961 - Flags: review?(mstange)
PS contains some state that is always valid, and some state that is only valid
when the profiler is active. This patch does the following.

- Splits PS into two parts for the two kinds of state: CorePS and ActivePS.

- Moves gPS (now split in two) into CorePS and ActivePS, as static instances,
  to improve encapsulation. This required changing all the state getters and
  setters into static methods.

- Existence tests for CorePS and ActivePS are now done via the Exists()
  methods.

Advantages of this change:

- It's now clear which parts of the global state (most of it!) are valid only
  when the profiler is active, and we don't have to invalidate those parts with
  zero/null/false when the profiler stops.

- Better OOP: more use of constructors and destructors, and more |const| to
  indicate what state is immutable.

- With the old code there were some places where the order of things required
  care, but with the new code it's not possible to get the order wrong.
Attachment #8859962 - Flags: review?(mstange)
Apologies for the fact that part 7 is so big and hard to review :/
Attachment #8859956 - Flags: review?(mstange) → review+
Attachment #8859957 - Flags: review?(mstange) → review+
Comment on attachment 8859958 [details] [diff] [review]
(part 3) - Don't measure process creation time on every profiler_start() call

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

::: tools/profiler/core/platform.cpp
@@ +226,5 @@
>  
>    #undef GET_AND_SET
>  
>  private:
>    // When profiler_init() or profiler_start() was most recently called.

This comment has always been wrong, then? Please fix :)

@@ +1467,5 @@
>      //   b.NameValue(marker, "location", ...);
>      if (mPayload) {
>        aWriter.StartObjectElement();
>        {
> +          mPayload->StreamPayload(aWriter, aProcessStartTime, aUniqueStacks);

Might want to fix the indent while you're here.
Attachment #8859958 - Flags: review?(mstange) → review+
Attachment #8859959 - Flags: review?(mstange) → review+
Comment on attachment 8859960 [details] [diff] [review]
(part 5) - Rename mThreadNameFilters as mFilters

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

okay.
Attachment #8859960 - Flags: review?(mstange) → review+
Attachment #8859961 - Flags: review?(mstange) → review+
Comment on attachment 8859962 [details] [diff] [review]
(part 7) - Split PS in two

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

::: tools/profiler/core/platform.cpp
@@ +143,4 @@
>  {
> +private:
> +  CorePS()
> +    : mProcessStartTime(mozilla::TimeStamp::ProcessCreation(mIgnore))

Why not move this into the constructor body so that you don't have to keep mIgnore around?

@@ +380,5 @@
> +  {
> +    // The Stop() call doesn't actually stop Run(); that happens in this
> +    // function's caller when the sampler thread is destroyed. Stop() just
> +    // gives the SamplerThread a chance to do some cleanup with gPSMutex
> +    // locked.

This comment is now confusing, because the samplerThread->Stop() call is in the caller of this function, very far away from the comment.

@@ +502,5 @@
> +  // can destroy it.
> +  SamplerThread* const mSamplerThread;
> +
> +  // The interposer that records main thread I/O.
> +  const UniquePtr<mozilla::ProfilerIOInterposeObserver> mInterposeObserver;

Thanks for switching this to UniquePtr.

@@ +2413,5 @@
>    MOZ_RELEASE_ASSERT(NS_IsMainThread());
> +  MOZ_RELEASE_ASSERT(CorePS::Exists() && !ActivePS::Exists(aLock));
> +
> +  // Fall back to the default values if the passed-in values are unreasonable.
> +  int entries     = aEntries  > 0 ? aEntries  : PROFILE_DEFAULT_ENTRIES;

I seriously question the value of aligning this line with the next one... your call though.
Attachment #8859962 - Flags: review?(mstange) → review+
> > +private:
> > +  CorePS()
> > +    : mProcessStartTime(mozilla::TimeStamp::ProcessCreation(mIgnore))
> 
> Why not move this into the constructor body so that you don't have to keep
> mIgnore around?

Because then mProcessStartTime can't be |const|. I'll add a comment explaining this. (And I might do a follow-up making that argument to ProcessCreation() optional, because it's not used in most calls.)


> > +    // The Stop() call doesn't actually stop Run(); that happens in this
> > +    // function's caller when the sampler thread is destroyed. Stop() just
> > +    // gives the SamplerThread a chance to do some cleanup with gPSMutex
> > +    // locked.
> 
> This comment is now confusing, because the samplerThread->Stop() call is in
> the caller of this function, very far away from the comment.

Oh, good catch. I'll move the comment back to where the Stop() call is :)
> (And I might do a follow-up making that argument to
> ProcessCreation() optional, because it's not used in most calls.)

Done: bug 1358320.
I will wait for bug 1357298 to land before landing part 7 in this bug.
sorry had to back this out for Assertion failure: NS_IsMainThread like https://treeherder.mozilla.org/logviewer.html#?job_id=93625319&repo=mozilla-inbound&lineNumber=12837
Flags: needinfo?(n.nethercote)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/265931fd5e5a
Backed out 6 changesets for Assertion failure: NS_IsMainThread and valgrind failures
Hi nicholas,

i wanted to re-checkin this changes because they were innocent but in part 2 i run into:
1 out of 4 hunks FAILED -- saving rejects to file tools/profiler/core/platform.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh unexport-ProfilerState

could you take a look, thanks!
Flags: needinfo?(n.nethercote)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8189f7d112e1de4d39580528ceda1c5c14ce5f50
Bug 1358074 (part 1, attempt 2) - Fix some bad indentation in platform.cpp. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/faf5feeeeeab81d44aa7a556644cd0aae3ec36f4
Bug 1358074 (part 2, attempt 2) - Unexport and rename ProfilerState. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1edbf89fe0c4601cbf0d19387b823212b1311313
Bug 1358074 (part 3, attempt 2) - Don't measure process creation time on every profiler_start() call. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc97f17420ef2d6e8cac44c6269f4cd8d3a77f3
Bug 1358074 (part 4, attempt 2) - Rename PS::{Mutex,AutoLock,LockRef} as PS{Mutex,AutoLock,LockRef}. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e764baa1533436e3a3b9195edaff43b9e1bcfec1
Bug 1358074 (part 5, attempt 2) - Rename mThreadNameFilters as mFilters. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e34e56f5e7adb50f3827125c25f121170ed071b
Bug 1358074 (part 6, attempt 2) - Don't use mWasPaused on Android. r=mstange.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Version: Trunk → 55 Branch
You need to log in before you can comment on or make changes to this bug.