Last Comment Bug 853358 - Add plugin profiling support
: Add plugin profiling support
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla23
Assigned To: Benoit Girard (:BenWa)
:
:
Mentors:
Depends on: 866239 874658 920909
Blocks: 921301 865236
  Show dependency treegraph
 
Reported: 2013-03-21 02:20 PDT by Benoit Girard (:BenWa)
Modified: 2013-09-26 18:23 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (14.56 KB, patch)
2013-03-21 02:22 PDT, Benoit Girard (:BenWa)
ehsan: review-
Details | Diff | Splinter Review
patch (21.32 KB, patch)
2013-03-26 08:15 PDT, Benoit Girard (:BenWa)
ehsan: review+
benjamin: review+
Details | Diff | Splinter Review
patch rebased (21.42 KB, patch)
2013-04-09 13:10 PDT, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
patch rebased + warning fix (21.96 KB, patch)
2013-04-09 14:03 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
patch rebased again (21.26 KB, patch)
2013-04-23 20:42 PDT, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
patch rebased again (21.27 KB, patch)
2013-04-24 10:59 PDT, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2013-03-21 02:20:02 PDT
Currently when we want to do multi-process profiling we contact and signal each process manually and run a script to combine the profiling. It should be easy to use an IPC message to do this ourselves.
Comment 1 Benoit Girard (:BenWa) 2013-03-21 02:22:01 PDT
Created attachment 727571 [details] [diff] [review]
patch

Asking ehsan for profiler changes, benjamin for plugins.
Comment 2 :Ehsan Akhgari 2013-03-21 13:25:04 PDT
Comment on attachment 727571 [details] [diff] [review]
patch

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +1738,5 @@
> +PluginProfilerObserver::Observe(nsISupports *aSubject,
> +                                const char *aTopic,
> +                                const PRUnichar *aData)
> +{
> +    ProfileSaveEvent* pse = (ProfileSaveEvent*)aSubject;

No, you cannot guarantee that this will be safe since anyone can send this notification.  Please add an XPCOM interface and implement it properly.

::: tools/profiler/GeckoProfilerImpl.h
@@ +295,5 @@
>    }
>    stack->addMarker(aMarker);
>  }
>  
> +class ProfileSaveEvent MOZ_FINAL : public nsISupports {

Please move this to TableTicker.cpp.

::: tools/profiler/TableTicker.cpp
@@ +167,5 @@
> +  JSAObjectBuilder* mBuilder;
> +  JSCustomArray* mThreads;
> +};
> +
> +void subprocess_callback(const char* aProfile, void* aClosure)

Nit: SubProcessCallback

@@ +207,5 @@
> +  closure.mThreads = threads;
> +
> +  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +  if (os) {
> +    os->NotifyObservers(new ProfileSaveEvent(subprocess_callback, &closure), "profiler-subprocess", nullptr);

ProfileSaveEvent is an XPCOM class, so you should hold on to it in a local nsRefPtr.  Otherwise, if an Observe implementation puts it into an nsCOMPtr, for example, the object will be destroyed prematurely when that nsCOMPtr is destroyed (since Release gets called which gets the refcount to 0.)
Comment 3 Benoit Girard (:BenWa) 2013-03-26 08:15:44 PDT
Created attachment 729556 [details] [diff] [review]
patch
Comment 4 :Ehsan Akhgari 2013-03-26 08:42:28 PDT
Comment on attachment 729556 [details] [diff] [review]
patch

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

Can you please ask somebody else to look at the plugin changes?  I'll try to get to this review some time this week but that may not be very likely.  Sorry. :(
Comment 5 Benoit Girard (:BenWa) 2013-03-26 09:20:40 PDT
Comment on attachment 729556 [details] [diff] [review]
patch

Yes, I just wanted to let you get a first pass before asking for the plugin bits to be reviewed.
Comment 6 Benjamin Smedberg [:bsmedberg] 2013-03-29 09:11:52 PDT
Comment on attachment 729556 [details] [diff] [review]
patch

I wonder if it wouldn't make more sense to write the profile to a temporary file. But either way, r=me on the plugin bits
Comment 7 Benoit Girard (:BenWa) 2013-04-01 07:55:14 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Comment on attachment 729556 [details] [diff] [review]
> patch
> 
> I wonder if it wouldn't make more sense to write the profile to a temporary
> file.

Have the plugin process write it's data to a file and have the parent process load it back up? It feels like that would be more flaky.
Comment 8 :Ehsan Akhgari 2013-04-03 07:11:36 PDT
Comment on attachment 729556 [details] [diff] [review]
patch

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

::: tools/profiler/Makefile.in
@@ +47,5 @@
>    platform.cpp \
>    nsProfilerFactory.cpp \
>    nsProfiler.cpp \
>    TableTicker.cpp \
> +	SaveProfileTask.cpp \

Nit: use spaces please.
Comment 9 Benoit Girard (:BenWa) 2013-04-09 13:10:35 PDT
Created attachment 735365 [details] [diff] [review]
patch rebased
Comment 10 Benoit Girard (:BenWa) 2013-04-09 14:03:23 PDT
Created attachment 735401 [details] [diff] [review]
patch rebased + warning fix
Comment 11 Benoit Girard (:BenWa) 2013-04-09 14:04:31 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f438836e66a4
Comment 12 Benoit Girard (:BenWa) 2013-04-09 14:45:27 PDT
(patch queue had a bad patch inside of it):
https://tbpl.mozilla.org/?tree=Try&rev=0f89c96118c1
Comment 13 Benoit Girard (:BenWa) 2013-04-23 20:42:55 PDT
Created attachment 741150 [details] [diff] [review]
patch rebased again

non trivial rebase again:
https://tbpl.mozilla.org/?tree=Try&rev=c90e6080f55a
Comment 14 Benoit Girard (:BenWa) 2013-04-24 10:59:08 PDT
Created attachment 741427 [details] [diff] [review]
patch rebased again

https://tbpl.mozilla.org/?tree=Try&rev=385d70d26ebb
Comment 15 Benoit Girard (:BenWa) 2013-04-24 19:30:16 PDT
Comment on attachment 741427 [details] [diff] [review]
patch rebased again

I never got review on the plugin part.
Comment 16 Benoit Girard (:BenWa) 2013-04-25 06:37:26 PDT
Comment on attachment 741427 [details] [diff] [review]
patch rebased again

Didn't see that I already got review.
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-04-25 18:57:17 PDT
https://hg.mozilla.org/mozilla-central/rev/c759d3eb1118

Note You need to log in before you can comment on or make changes to this bug.