Add plugin profiling support

RESOLVED FIXED in mozilla23

Status

()

Core
Gecko Profiler
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks: 1 bug)

unspecified
mozilla23
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 727571 [details] [diff] [review]
patch

Asking ehsan for profiler changes, benjamin for plugins.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #727571 - Flags: review?(benjamin)
(Assignee)

Updated

4 years ago
Attachment #727571 - Flags: review?(ehsan)
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.)
Attachment #727571 - Flags: review?(ehsan) → review-
(Assignee)

Comment 3

4 years ago
Created attachment 729556 [details] [diff] [review]
patch
Attachment #727571 - Attachment is obsolete: true
Attachment #727571 - Flags: review?(benjamin)
Attachment #729556 - Flags: review?(ehsan)
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. :(
(Assignee)

Comment 5

4 years ago
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.
Attachment #729556 - Flags: review?(benjamin)
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
Attachment #729556 - Flags: review?(benjamin) → review+
(Assignee)

Comment 7

4 years ago
(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 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.
Attachment #729556 - Flags: review?(ehsan) → review+
(Assignee)

Comment 9

4 years ago
Created attachment 735365 [details] [diff] [review]
patch rebased
Attachment #729556 - Attachment is obsolete: true
Attachment #735365 - Flags: review+
(Assignee)

Comment 10

4 years ago
Created attachment 735401 [details] [diff] [review]
patch rebased + warning fix
Attachment #735365 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=f438836e66a4
(Assignee)

Comment 12

4 years ago
(patch queue had a bad patch inside of it):
https://tbpl.mozilla.org/?tree=Try&rev=0f89c96118c1
(Assignee)

Comment 13

4 years ago
Created attachment 741150 [details] [diff] [review]
patch rebased again

non trivial rebase again:
https://tbpl.mozilla.org/?tree=Try&rev=c90e6080f55a
Attachment #735401 - Attachment is obsolete: true
Attachment #741150 - Flags: review+
(Assignee)

Comment 14

4 years ago
Created attachment 741427 [details] [diff] [review]
patch rebased again

https://tbpl.mozilla.org/?tree=Try&rev=385d70d26ebb
Attachment #741150 - Attachment is obsolete: true
Attachment #741427 - Flags: review+
(Assignee)

Comment 15

4 years ago
Comment on attachment 741427 [details] [diff] [review]
patch rebased again

I never got review on the plugin part.
Attachment #741427 - Flags: review?(benjamin)
(Assignee)

Comment 16

4 years ago
Comment on attachment 741427 [details] [diff] [review]
patch rebased again

Didn't see that I already got review.
Attachment #741427 - Flags: review?(benjamin)
(Assignee)

Comment 17

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c759d3eb1118
https://hg.mozilla.org/mozilla-central/rev/c759d3eb1118
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Updated

4 years ago
Depends on: 866239
Depends on: 874658
(Assignee)

Updated

4 years ago
Blocks: 865236
(Assignee)

Updated

4 years ago
Depends on: 920909
(Assignee)

Updated

4 years ago
Blocks: 921301
You need to log in before you can comment on or make changes to this bug.