Closed Bug 1340928 Opened 7 years ago Closed 7 years ago

Still more profiler cleanups

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

(16 files, 2 obsolete files)

3.91 KB, patch
mstange
: review+
Details | Diff | Splinter Review
6.03 KB, patch
mstange
: review+
Details | Diff | Splinter Review
1.23 KB, patch
mstange
: review+
Details | Diff | Splinter Review
4.21 KB, patch
mstange
: review+
Details | Diff | Splinter Review
9.68 KB, patch
mstange
: review+
Details | Diff | Splinter Review
11.74 KB, patch
mstange
: review+
Details | Diff | Splinter Review
10.59 KB, patch
mstange
: review+
Details | Diff | Splinter Review
9.17 KB, patch
mstange
: review+
Details | Diff | Splinter Review
6.90 KB, patch
mstange
: review+
Details | Diff | Splinter Review
3.39 KB, patch
baku
: review+
Details | Diff | Splinter Review
19.89 KB, patch
mstange
: review+
Details | Diff | Splinter Review
4.22 KB, patch
mstange
: review+
Details | Diff | Splinter Review
2.81 KB, patch
mstange
: review+
Details | Diff | Splinter Review
2.93 KB, patch
mstange
: review+
Details | Diff | Splinter Review
2.14 KB, patch
mstange
: review+
Details | Diff | Splinter Review
26.23 KB, patch
mstange
: review+
Details | Diff | Splinter Review
I'm working on a big patch that overhauls the handling of all the global state in platform.cpp so that it's properly and obviously synchronized. It's a big change so I'm pulling out precursor changes where possible, and this bug is for them.
- Don't bother checking gSampler in ProfilerSignalHandler. It is equivalent
  to checking gIsActive and we do that at the top of the loop in
  SignalSender(). There is no point repeatedly checking the same condition in
  the middle of that loop; that just opens up the possibility of partially
  complete samples where some threads are missing.

- Clear gCurrentThreadInfo in SignalSender() instead of in
  ProfilerSignalHandler(). The effect is much the same, but this change means
  gCurrentThreadInfo is both set and cleared in SignalSender(), i.e. on a
  single thread, removing any need for Atomic<>.
Attachment #8838989 - Flags: review?(mstange)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
This change means that all the relevant code is now within
platform-linux-android.cpp, which is nice.
Attachment #8838990 - Flags: review?(mstange)
There is another PlatformStop() call earlier in the function, and gIsActive is
always false by the time we reach the removed call, so it's dead code.
Attachment #8838991 - Flags: review?(mstange)
It's only being used in a boolean fashion, so this patch replaces it with a
boolean.
Attachment #8838992 - Flags: review?(mstange)
This avoids the need for platform-linux-android.cpp to read gInterval off the
main thread in an awkward spot. It also makes platform-linux-android.cpp
more like platform-{win32,macos}.cpp.
Attachment #8838993 - Flags: review?(mstange)
This patch mostly does formatting fixes.

It also removes some declarations from platform.h that are no longer necessary
now that platform-linux-android.cpp is in the same compilation unit as
platform.cpp (due to it being #include-d directly); this required reordering
some things.
Attachment #8838994 - Flags: review?(mstange)
PlatformStart() and PlatformStop() are currently responsible for setting and
clearing gIsActive, but it's better if we do it in profiler_{start,stop}().

The patch also does the following.

- Adds some missing emacs/vim modelines.

- Makes Platform{Start,Stop}() crash if they have failures. I'm not at all
  confident that ignoring the errors as is currently done will result in
  sensible behaviour, so brittleness is better.
Attachment #8838995 - Flags: review?(mstange)
The profiler can currently handle nested calls to profiler_{init,shutdown}() --
only the first call to profiler_init() and the last call to profiler_shutdown()
do anything. And sure enough, we have the following.

- Outer init/shutdown pairs in XRE_main()/XRE_InitChildProcess() (via
  GeckoProfilerInitRAII).

- Inner init/shutdown pairs in NS_InitXPCOM2()/NS_InitMinimalXPCOM() (both shut
  down in ShutdownXPCOM()).

This is all rather silly, so the patch removes the inner pairs. This will allow
gInitCount -- which tracks the nesting depth -- to be removed in a future patch.
Attachment #8838996 - Flags: review?(mstange)
Comment on attachment 8838996 [details] [diff] [review]
(part 8) - Remove nested calls to profiler_{init,shutdown}()

Turns out I need to add a GeckoProfilerInitRAII to XRE_XPCShellMain() and I haven't yet worked out the right place to put it. Tomorrow.
Attachment #8838996 - Flags: review?(mstange)
Comment on attachment 8838996 [details] [diff] [review]
(part 8) - Remove nested calls to profiler_{init,shutdown}()

I'm not sure about this patch so I'm going to abandon it for now.
Attachment #8838996 - Attachment is obsolete: true
This removes the one use of gStartTime outside of platform*.cpp, which lets us
restrict its visibility to just that compilation unit.
Attachment #8839281 - Flags: review?(mstange)
It's used to set gUnwindStackScan, which is never used.
Attachment #8839302 - Flags: review?(mstange)
Ok, I worked out where to do the init/shutdown for xpcshell.
Attachment #8839315 - Flags: review?(mstange)
It looks like it used to be called by multiple constructors, but that's no
longer the case.
Attachment #8839325 - Flags: review?(amarchesini)
Comment on attachment 8839325 [details] [diff] [review]
(part 11) - Inline and remove ContentParent::InitializeMembers

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

::: dom/ipc/ContentParent.cpp
@@ +1910,1 @@
>  

remove this empty line here.
Attachment #8839325 - Flags: review?(amarchesini) → review+
profiler_get_gatherer() exposes ProfileGatherer to the outside world in a way
that makes future changes difficult.

This patch:

- Removes ProfileGatherer.h from the list of headers exported from the
  profiler.

- Removes nsIProfiler.profileGatherer and nsProfiler::GetProfileGatherer().

- Replaces profiler_get_gatherer() with three new functions that provide
  minimal but sufficient access to ProfileGatherer:
  profiler_will_gather_OOP_profile(), profiler_gathered_OOP_profile(), and
  profiler_OOP_exit_profile().

  These functions provide access to the ProfileGatherer in a similar fashion to
  the pre-existing functions profiler_get_profile_jsobject_async() and
  profiler_save_profile_to_file_async()

This significantly reduces the size of the profiler's API surface.
Attachment #8839369 - Flags: review?(mstange)
This is the correct version of the patch.
Attachment #8839725 - Flags: review?(mstange)
The |nsIFile*| one is only called by the |const nsACString&| one, so this patch
combines them.
Attachment #8839753 - Flags: review?(mstange)
It has a single call site.
Attachment #8839784 - Flags: review?(mstange)
Attachment #8838989 - Flags: review?(mstange) → review+
Attachment #8838990 - Flags: review?(mstange) → review+
Attachment #8838991 - Flags: review?(mstange) → review+
Attachment #8838992 - Flags: review?(mstange) → review+
Attachment #8838993 - Flags: review?(mstange) → review+
Comment on attachment 8838994 [details] [diff] [review]
(part 6) - Clean up profiler code relating to env vars

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

::: tools/profiler/core/platform.cpp
@@ +1253,5 @@
>  
>  enum class ProfilerVerbosity : int8_t { UNCHECKED, NOTVERBOSE, VERBOSE };
>  
>  // Raced on, potentially
>  static ProfilerVerbosity profiler_verbosity = ProfilerVerbosity::UNCHECKED;

This should probably be renamed to gProfilerVerbosity.
Attachment #8838994 - Flags: review?(mstange) → review+
> This should probably be renamed to gProfilerVerbosity.

I changed it to gVerbosity, and renamed the type ProfilerVerbosity to Verbosity. In both cases the "Profiler" was obvious :)
Attachment #8838995 - Flags: review?(mstange) → review+
Attachment #8839281 - Flags: review?(mstange) → review+
Attachment #8839302 - Flags: review?(mstange) → review+
Attachment #8839315 - Flags: review?(mstange) → review+
Comment on attachment 8839369 [details] [diff] [review]
(part 12) - Remove profiler_get_gatherer()

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

Looks good. I was afraid that it was going to make it harder to match up overlapping asynchronous profile collections, but the old API wasn't any better at handling that case, so this is fine.
Attachment #8839369 - Flags: review?(mstange) → review+
Attachment #8839315 - Attachment is obsolete: true
Attachment #8839315 - Flags: review+
Comment on attachment 8839725 [details] [diff] [review]
(part 10) - Remove nested calls to profiler_{init,shutdown}()

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

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +1655,5 @@
>      if (CrashReporter::GetEnabled())
>          CrashReporter::UnsetExceptionHandler();
>  #endif
>  
> +    profiler_shutdown();    // This must precede NS_LogTerm().

Do you know why? Might be worth adding a comment.
Attachment #8839725 - Flags: review?(mstange) → review+
Attachment #8839753 - Flags: review?(mstange) → review+
Comment on attachment 8839755 [details] [diff] [review]
(part 14) - Factor out code repeated in both ProfileGatherer::Start() methods

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

::: tools/profiler/gecko/ProfileGatherer.h
@@ +27,5 @@
>  private:
>    ~ProfileGatherer() {};
>    void Finish();
>    void Reset();
> +  void Start2(double aSinceTime);

StartImpl? I don't have a strong opinion on it though.
Attachment #8839755 - Flags: review?(mstange) → review+
Attachment #8839784 - Flags: review?(mstange) → review+
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4aa11ecb4d6873c113abf48f0694c9da8150d1
Bug 1340928 (part 7) - Factor out gIsActive handling in platform-*.cpp. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac2f3a144257a917ac118499f54c6e18862bd21
Bug 1340928 (part 8) - Pass gStartTime to DuplicateLastSample(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b681adb0173c3d41faec2e7ff1cea163b6fdb6
Bug 1340928 (part 9) - Remove MOZ_PROFILER_STACK_SCAN env var. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7e56055a3ec7b7b4bec6a5a1bc9265e0cb4a4d
Bug 1340928 (part 10) - Remove nested calls to profiler_{init,shutdown}(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6afc9b586b6f81b7f7bce2cff2e0cc1e8623282
Bug 1340928 (part 11) - Inline and remove ContentParent::InitializeMembers. r=baku.

https://hg.mozilla.org/integration/mozilla-inbound/rev/34b0a316c4130407a4b7bcaca4be8557945ddb9b
Bug 1340928 (part 12) - Remove profiler_get_gatherer(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f1cf49bed1d6466244b54d9d9b2483a1fc0627
Bug 1340928 (part 13) - Merge two ProfileGatherer::Start() variants. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0d5dcaa2c361b2b5bd66f9ff06292098305c726
Bug 1340928 (part 14) - Factor out code repeated in both ProfileGatherer::Start() methods. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/494906dbf2d99146cc25c27f8cb51f84c6fe4759
Bug 1340928 (part 15) - Inline and remove ToJSObject(). r=mstange.
This patch does the following.

- Uses "entries" consistently for the name of the value that is obtained from
  MOZ_PROFILER_ENTRIES and is the first argument to profiler_start(). (I.e. not
  "entry" or "entrySize".)

- Removes variables (e.g. PROFILER_HELP) holding env var names and uses the
  names (e.g. "MOZ_PROFILER_HELP") directly. Some of the names are already used
  directly and I think the slight repetition isn't harmful. It's unlikely that
  we'd want to change these names the way we might need to change a numeric
  value, and they're perfectly descriptive.

- Changes the "MOZ_PROFILING_FEATURES" string in the weird Android-only startup
  code to be "MOZ_PROFILER_FEATURES", for consistency.

- Renames gUnwindInterval and gProfileEntries as gEnvVarInterval and
  gEnvVarEntries to make it clearer that they come from environment variables,
  but otherwise are parallel to gInterval and gEntries.

- Puts entries before intervals in most places, to match the profiler_start()
  argument order.

- Changes profiler_usage() so that (a) it always prints, no matter the
  verbosity, (b) it exits at its end, and (c) doesn't double-print "Profiler: "
  at the start of each line.
Attachment #8840407 - Flags: review?(mstange)
Attachment #8840407 - Flags: review?(mstange) → review+
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d41302b8481
(part 16) - Clean up profiler env var handling. r=mstange.
That's enough patches for this bug.
Keywords: leave-open
Blocks: 1342306
https://hg.mozilla.org/mozilla-central/rev/4d41302b8481
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.