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 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 on attachment 572072 [details] [diff] [review] Interface stub Boring

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

Fold all 3 patches into: https://hg.mozilla.org/integration/mozilla-inbound/rev/18019396a173

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.

Fixed a mistake caught on try.

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 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

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

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 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 :)

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

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

(The make package fix didn't make Firefox 11, so don't know if aurora approval needed just for that?)

It's not required for 11, I wont be trying to uplift this.