Add a Profiler XPCOM Module

RESOLVED FIXED in mozilla12

Status

()

Core
Gecko Profiler
RESOLVED FIXED
6 years ago
a month ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Depends on: 1 bug)

Trunk
mozilla12
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1])

Attachments

(6 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 572072 [details] [diff] [review]
Interface stub
(Assignee)

Comment 2

6 years ago
Created attachment 572114 [details] [diff] [review]
Create component
(Assignee)

Updated

6 years ago
Blocks: 700754
(Assignee)

Comment 3

6 years ago
Created attachment 572935 [details] [diff] [review]
Create component
Assignee: nobody → bgirard
Attachment #572114 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 4

6 years ago
Created attachment 572964 [details] [diff] [review]
JSM Stub
(Assignee)

Updated

6 years ago
Attachment #572072 - Flags: review?(jmuizelaar)
(Assignee)

Updated

6 years ago
Attachment #572935 - Flags: review?(jmuizelaar)
(Assignee)

Updated

6 years ago
Attachment #572964 - Flags: review?(jmuizelaar)
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.
Attachment #572935 - Flags: review?(jmuizelaar) → review+
Comment on attachment 572072 [details] [diff] [review]
Interface stub

Boring
Attachment #572072 - Flags: review?(jmuizelaar) → review+
Attachment #572964 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

6 years ago
Blocks: 683229
(Assignee)

Comment 7

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

Comment 8

6 years ago
Fold all 3 patches into:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18019396a173
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/18019396a173
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

6 years ago
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.
Attachment #578921 - Flags: review?(jmuizelaar)
(Assignee)

Comment 11

6 years ago
Created attachment 578943 [details] [diff] [review]
Implementation + bug fix for mac port + android fix

Fixed a mistake caught on try.
Attachment #578921 - Attachment is obsolete: true
Attachment #578921 - Flags: review?(jmuizelaar)
Attachment #578943 - Flags: review?(jmuizelaar)
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?
Attachment #578943 - Flags: review?(jmuizelaar) → review+
Whiteboard: [Snappy:P1]
(Assignee)

Comment 13

6 years ago
Created attachment 579438 [details] [diff] [review]
Implement (review)
Attachment #578943 - Attachment is obsolete: true
Attachment #579438 - Flags: review?(jmuizelaar)
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

6 years ago
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

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

Comment 17

6 years ago
Created attachment 579772 [details] [diff] [review]
Implement (review)
Attachment #579438 - Attachment is obsolete: true
Attachment #579438 - Flags: review?(jmuizelaar)
Attachment #579772 - Flags: review?(jmuizelaar)
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 :)
Attachment #579772 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 19

6 years ago
Created attachment 579787 [details] [diff] [review]
Implement (fixes)
Attachment #579787 - Flags: review+
(Assignee)

Comment 20

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3695fe861c

Comment 21

6 years ago
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

6 years ago
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
https://hg.mozilla.org/mozilla-central/rev/ba3695fe861c
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

5 years ago
Created attachment 583213 [details] [diff] [review]
fix 'make package'
Attachment #583213 - Flags: review?(jmuizelaar)
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #583213 - Flags: review?(jmuizelaar) → review+
(The make package fix didn't make Firefox 11, so don't know if aurora approval needed just for that?)
(Assignee)

Comment 26

5 years ago
It's not required for 11, I wont be trying to uplift this.
https://hg.mozilla.org/mozilla-central/rev/e5f86ae7ef6a
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla11 → mozilla12
Blocks: 714255
Depends on: 743428
Depends on: 734335
No longer depends on: 743428
(Assignee)

Updated

4 years ago
Component: General → Gecko Profiler
Depends on: 1351091
You need to log in before you can comment on or make changes to this bug.