Closed Bug 1321617 Opened 8 years ago Closed 7 years ago

Should capture the parent process's profile at the point that getProfile is called, and not wait for the subprocess profiles to arrive

Categories

(Core :: Gecko Profiler, defect)

Product:
Core
Component:
Gecko Profiler
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mstange, Assigned: mstange)

Details

Attachments

(1 file, 6 obsolete files)

Bug 1321617 - Add a test for profiler_stream_json_for_this_process.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
      No description provided.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Comment on attachment 8856184 [details]
Bug 1321617 - Don't store profiles on the CPPC; pass them right to the ProfileGatherer.

https://reviewboard.mozilla.org/r/128112/#review130690
Attachment #8856184 - Flags: review?(n.nethercote) → review+
Comment on attachment 8856185 [details]
Bug 1321617 - Move parts of StreamJSON out into the callers and rename it to locked_profiler_stream_json_for_this_process.

https://reviewboard.mozilla.org/r/128114/#review130692
Attachment #8856185 - Flags: review?(n.nethercote) → review+
Comment on attachment 8856186 [details]
Bug 1321617 - Reduce the scope of the lock in profiler_get_profile.

https://reviewboard.mozilla.org/r/128116/#review130684

::: tools/profiler/core/platform.cpp:1426
(Diff revision 1)
>  }
>  
> +static bool
> +profiler_stream_json_for_this_process(SpliceableJSONWriter& aWriter, double aSinceTime)
> +{
> +  LOG("locked_profiler_stream_json_for_this_process");

Remove the "locked_" here.
Attachment #8856186 - Flags: review?(n.nethercote) → review+
Comment on attachment 8856187 [details]
Bug 1321617 - Call profiler_stream_json_for_this_process in ProfileGatherer::Finish() and get rid of the 'profile-subprocess' notification indirection.

https://reviewboard.mozilla.org/r/128118/#review130694
Attachment #8856187 - Flags: review?(n.nethercote) → review+
Comment on attachment 8856188 [details]
Bug 1321617 - Capture parent process profiles in ProfileGatherer::Start2, not in ProfileGatherer::Finish.

https://reviewboard.mozilla.org/r/128120/#review130688

::: tools/profiler/gecko/ProfileGatherer.cpp
(Diff revision 1)
> -      // profiler_start() have both been called. (If that does happen, we'll end up
> -      // with a franken-profile that includes a mix of data from the old and new
> -      // profile activations.) We could include the activity generation to detect
> -      // that, but it's not worth it for what should be an extremely unlikely case.
> -      // It would be better if this class was rearranged so that
> -      // profiler_get_profile() was called for the parent process in Start()

Note that I changed this comment to refer to Start2() instead of Start() before I landed the patches in bug 1350967. You'll probably get conflicts when you rebase and that will be why. Sorry... I wouldn't have made the change if I had seen this patch before I landed.
Attachment #8856188 - Flags: review?(n.nethercote) → review+
Comment on attachment 8856189 [details]
Bug 1321617 - Add a test for profiler_stream_json_for_this_process.

https://reviewboard.mozilla.org/r/128122/#review130680

::: tools/profiler/tests/gtest/GeckoProfiler.cpp:371
(Diff revision 1)
> +                 features, MOZ_ARRAY_LENGTH(features),
> +                 filters, MOZ_ARRAY_LENGTH(filters));
> +
> +  w.Start(SpliceableJSONWriter::SingleLineStyle);
> +  bool isActive = profiler_stream_json_for_this_process(w);
> +  ASSERT_TRUE(isActive);

Just call the function within the ASSERT_TRUE()? That's how it's done in the rest of the file.
Attachment #8856189 - Flags: review?(n.nethercote) → review+
Lovely changes! Lots of code removed, including a whole .idl file, and a new test.

My understanding of how the profiles are stitched together is weak, so this wasn't the most thorough review. With that caveat in mind, it all looks good to me.
Comment on attachment 8856183 [details]
Bug 1321617 - Tell the CrossProcessProfilerController whether a profile is a response to a GatherProfile request or whether it was sent because the process was exiting.

https://reviewboard.mozilla.org/r/128110/#review131110

Makes sense to me! Thanks!

::: dom/plugins/ipc/PluginModuleParent.cpp:3265
(Diff revision 1)
>  
>  #endif // MOZ_CRASHREPORTER_INJECTOR
>  
>  mozilla::ipc::IPCResult
> -PluginModuleChromeParent::RecvProfile(const nsCString& aProfile)
> +PluginModuleChromeParent::RecvProfile(const nsCString& aProfile,
> +                                      const bool&  aIsExitProfile)

Nit - extra space after bool&.

::: gfx/ipc/GPUParent.cpp:431
(Diff revision 1)
>    UniquePtr<char[]> profile = profiler_get_profile();
>    if (profile) {
>      profileCString = nsDependentCString(profile.get());
>    }
>  
> -  Unused << SendProfile(profileCString);
> +  Unused << SendProfile(profileCString, false);

Feel free to push back on this, but especially when we're passing primitive values to or from a function, I like to have explainer comments, like:

```C++
Unused << SendProfile(profileCString, false /* aIsExitProfile */);
```

To kinda hint at what the value means. Something you might want to consider before landing, but I won't push it.
Attachment #8856183 - Flags: review?(mconley) → review+
Attachment #8856183 - Attachment is obsolete: true
Attachment #8856184 - Attachment is obsolete: true
Attachment #8856185 - Attachment is obsolete: true
Attachment #8856186 - Attachment is obsolete: true
Attachment #8856187 - Attachment is obsolete: true
Attachment #8856188 - Attachment is obsolete: true
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24a0b0a853ea
Tell the CrossProcessProfilerController whether a profile is a response to a GatherProfile request or whether it was sent because the process was exiting. r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7e2505e2d24
Don't store profiles on the CPPC; pass them right to the ProfileGatherer. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/63d83cce024c
Move parts of StreamJSON out into the callers and rename it to locked_profiler_stream_json_for_this_process. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f285b957e2
Reduce the scope of the lock in profiler_get_profile. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/c918b1d4df17
Call profiler_stream_json_for_this_process in ProfileGatherer::Finish() and get rid of the 'profile-subprocess' notification indirection. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/f989f9e01a28
Capture parent process profiles in ProfileGatherer::Start2, not in ProfileGatherer::Finish. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/853ace07ede9
Add a test for profiler_stream_json_for_this_process. r=njn
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: