Last Comment Bug 699918 - Add a Profiler XPCOM Module
: Add a Profiler XPCOM Module
Status: RESOLVED FIXED
[Snappy:P1]
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on: 734335
Blocks: 683229 700754 714255
  Show dependency treegraph
 
Reported: 2011-11-04 14:06 PDT by Benoit Girard (:BenWa)
Modified: 2013-06-05 09:11 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Interface stub (12.18 KB, patch)
2011-11-04 14:08 PDT, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
Create component (4.54 KB, patch)
2011-11-04 15:45 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Create component (4.14 KB, patch)
2011-11-08 12:10 PST, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
JSM Stub (2.69 KB, patch)
2011-11-08 12:49 PST, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
Implementation + bug fix for mac port (22.41 KB, patch)
2011-12-04 11:05 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Implementation + bug fix for mac port + android fix (22.58 KB, patch)
2011-12-04 14:58 PST, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
Implement (review) (22.51 KB, patch)
2011-12-06 13:30 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Implement (review) (22.49 KB, patch)
2011-12-07 11:48 PST, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
Implement (fixes) (22.49 KB, patch)
2011-12-07 12:13 PST, Benoit Girard (:BenWa)
bgirard: review+
Details | Diff | Splinter Review
fix 'make package' (815 bytes, patch)
2011-12-20 10:33 PST, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2011-11-04 14:06:49 PDT
We need to expose the profiler feature to allow extensions to programmatically control the profiler. The goal is to correlate profile with each TP5 test page load as well as reporting responsiveness problems to the user with profile data.
Comment 1 Benoit Girard (:BenWa) 2011-11-04 14:08:58 PDT
Created attachment 572072 [details] [diff] [review]
Interface stub
Comment 2 Benoit Girard (:BenWa) 2011-11-04 15:45:58 PDT
Created attachment 572114 [details] [diff] [review]
Create component
Comment 3 Benoit Girard (:BenWa) 2011-11-08 12:10:31 PST
Created attachment 572935 [details] [diff] [review]
Create component
Comment 4 Benoit Girard (:BenWa) 2011-11-08 12:49:59 PST
Created attachment 572964 [details] [diff] [review]
JSM Stub
Comment 5 Jeff Muizelaar [:jrmuizel] 2011-11-09 13:57:01 PST
Comment on attachment 572935 [details] [diff] [review]
Create component

># HG changeset patch
># User Benoit Girard <b56girard@gmail.com>
># Date 1320722715 18000
># Node ID ff55bc1ec039ab61d2d8dca9a2f65691c39a4faf
># Parent  ef4b6fc31a5503dc1ad00a9ec9addfd83d38c02e
>imported patch Component
>
>diff --git a/toolkit/library/libxul-config.mk b/toolkit/library/libxul-config.mk
>--- a/toolkit/library/libxul-config.mk
>+++ b/toolkit/library/libxul-config.mk
>@@ -263,19 +263,17 @@ COMPONENT_LIBS += imgicon
> endif
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),android)
> COMPONENT_LIBS += widget_android
> endif
> 
> STATIC_LIBS += thebes ycbcr
> 
>-ifeq ($(MOZ_WIDGET_TOOLKIT),android)
> STATIC_LIBS += profiler
>-endif

extra component.
Comment 6 Jeff Muizelaar [:jrmuizel] 2011-11-09 13:57:25 PST
Comment on attachment 572072 [details] [diff] [review]
Interface stub

Boring
Comment 7 Benoit Girard (:BenWa) 2011-11-25 15:00:49 PST
I get the following build error on tinderbox but not my local build:
i686-apple-darwin9-g++-4.2.1: ../../staticlib/libprofiler.a: No such file or directory
Comment 8 Benoit Girard (:BenWa) 2011-11-27 21:36:11 PST
Fold all 3 patches into:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18019396a173
Comment 9 Marco Bonardo [::mak] 2011-11-28 05:30:31 PST
https://hg.mozilla.org/mozilla-central/rev/18019396a173
Comment 10 Benoit Girard (:BenWa) 2011-12-04 11:05:24 PST
Created attachment 578921 [details] [diff] [review]
Implementation + bug fix for mac port

Expose the functionality needed for the demo add-on I have. This fixes bug with the mac port. I have rebased the windows port patch on top of this+mac port and everything works well together.
Comment 11 Benoit Girard (:BenWa) 2011-12-04 14:58:56 PST
Created attachment 578943 [details] [diff] [review]
Implementation + bug fix for mac port + android fix

Fixed a mistake caught on try.
Comment 12 Jeff Muizelaar [:jrmuizel] 2011-12-05 13:27:54 PST
Comment on attachment 578943 [details] [diff] [review]
Implementation + bug fix for mac port + android fix

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

::: modules/libpref/src/init/all.js
@@ +3405,5 @@
> +// SPS Profiler
> +pref("profiler.enabled", false);
> +pref("profiler.interval", 10);
> +pref("profiler.entries", 100000);
> +

Do we use these prefs?

::: toolkit/content/license.html
@@ +2258,5 @@
>        <span class="path">widget/src/cocoa/GfxInfo.mm</span>
>        and also some files in the directories
>        <span class="path">ipc/chromium/</span>,
>        <span class="path">dom/plugins/</span>,
> +      <span class="path">tools/profiler/sps/</span>,

This should really be a separate patch...

::: tools/profiler/sps/Makefile.in
@@ -58,5 @@
> -
> -EXPORTS         := $(addprefix $(srcdir)/, $(EXPORTS))
> -
> -include $(topsrcdir)/config/rules.mk
> -

Not sure why this is here.

::: tools/profiler/sps/TableTicker.cpp
@@ +48,5 @@
>  #include "nsThreadUtils.h"
>  #include "prenv.h"
>  
> +using std::string;
> +

I'm not sure if we should be using stl strings here...

@@ +456,5 @@
> +      //}
> +      //sResponsivenessLoc = 0;
> +    }
> +    TimeDuration delta = aTime - sLastTracerEvent;
> +    sResponsivenessTimes[sResponsivenessLoc++] = (uint32_t)(delta.ToMilliseconds());

Do we really need to truncate these to uint32_t can't we just leave them as doubles?

::: tools/profiler/sps/platform-macos.cc
@@ +184,5 @@
>      ScopedLock lock(mutex_);
>      SamplerRegistry::AddActiveSampler(sampler);
>      if (instance_ == NULL) {
>        instance_ = new SamplerThread(sampler->interval());
> +      printf("start sampler thread\n");

intentional?

@@ +212,5 @@
>      while (SamplerRegistry::sampler->IsActive()) {
>        SampleContext(SamplerRegistry::sampler);
>        OS::Sleep(interval_);
>      }
> +    printf("shutdown\n");

Intentional?
Comment 13 Benoit Girard (:BenWa) 2011-12-06 13:30:35 PST
Created attachment 579438 [details] [diff] [review]
Implement (review)
Comment 14 :Felipe Gomes (needinfo me!) 2011-12-06 18:02:39 PST
Comment on attachment 579438 [details] [diff] [review]
Implement (review)

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

::: tools/profiler/nsProfiler.cpp
@@ +67,5 @@
> +NS_IMETHODIMP
> +nsProfiler::GetProfile(char **aProfile)
> +{
> +  char *profile = SAMPLER_GET_PROFILE();
> +  uint32_t len = strlen(profile);

I've changed this last uint32_t to PRUint32 in my patch but I wonder if you'd like to land it with PRUint32 already

@@ +71,5 @@
> +  uint32_t len = strlen(profile);
> +  char *profileStr = static_cast<char *>
> +                       (nsMemory::Clone(profile, len * sizeof(char)));
> +  *aProfile = profileStr;
> +  free(profile);

if SAMPLER_GET_PROFILE() returned the static "" (due to the profiler not being active), this free will cause an access violation
Comment 15 Mozilla RelEng Bot 2011-12-07 10:25:40 PST
Try run for c0798fce1793 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c0798fce1793
Results (out of 144 total builds):
    exception: 2
    success: 121
    warnings: 13
    failure: 8
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-c0798fce1793
Comment 16 Mozilla RelEng Bot 2011-12-07 10:55:16 PST
Try run for c3327b684659 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c3327b684659
Results (out of 189 total builds):
    exception: 2
    success: 167
    warnings: 17
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-c3327b684659
Comment 17 Benoit Girard (:BenWa) 2011-12-07 11:48:52 PST
Created attachment 579772 [details] [diff] [review]
Implement (review)
Comment 18 Jeff Muizelaar [:jrmuizel] 2011-12-07 12:00:32 PST
Comment on attachment 579772 [details] [diff] [review]
Implement (review)

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

::: tools/profiler/sps/sps_sampler.h
@@ +46,1 @@
>  

I don't think we need to included these here and I would rather not.

@@ +84,5 @@
> +void mozilla_sampler_start(int aEntries, int aInterval);
> +void mozilla_sampler_stop();
> +bool mozilla_sampler_is_active();
> +void mozilla_sampler_responsiveness(TimeStamp time);
> +const float* mozilla_sampler_get_responsiveness();

Use double instead of float please :)
Comment 19 Benoit Girard (:BenWa) 2011-12-07 12:13:01 PST
Created attachment 579787 [details] [diff] [review]
Implement (fixes)
Comment 21 Mozilla RelEng Bot 2011-12-07 17:10:50 PST
Try run for c3327b684659 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c3327b684659
Results (out of 189 total builds):
    exception: 2
    success: 167
    warnings: 17
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-c3327b684659
Comment 22 Mozilla RelEng Bot 2011-12-07 17:11:37 PST
Try run for c0798fce1793 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c0798fce1793
Results (out of 144 total builds):
    exception: 2
    success: 121
    warnings: 13
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-c0798fce1793
Comment 23 Ed Morley [:emorley] 2011-12-08 08:53:35 PST
https://hg.mozilla.org/mozilla-central/rev/ba3695fe861c
Comment 24 Benoit Girard (:BenWa) 2011-12-20 10:33:19 PST
Created attachment 583213 [details] [diff] [review]
fix 'make package'
Comment 25 Ed Morley [:emorley] 2011-12-20 11:10:41 PST
(The make package fix didn't make Firefox 11, so don't know if aurora approval needed just for that?)
Comment 26 Benoit Girard (:BenWa) 2011-12-20 11:16:55 PST
It's not required for 11, I wont be trying to uplift this.
Comment 27 Ed Morley [:emorley] 2011-12-21 04:32:13 PST
https://hg.mozilla.org/mozilla-central/rev/e5f86ae7ef6a

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