Bug 24253 – Add functions to ChromiumBridge for profiler API control

• Component: Platform
• Status: RESOLVED WONTFIX

[Antony Sargent]

patch forthcoming

[Antony Sargent]

Created attachment 28105 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 28105 [details]
Patch

>Index: ChangeLog

>[phone]  Antony Sargent  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add functions to ChromiumBridge for profiler API control (eg Quantify) 
>+
>+        * platform/chromium/ChromiumBridge.h:

nit: please include a bug link in the ChangeLog.


>Index: platform/chromium/ChromiumBridge.h

>+        // PerformanceProfiler ------------------------------------------------
>+        static void performanceProfilerStart();

nit: wouldn't it be sufficient to just use the prefix "profiler"?  usually a
profiler has to do with performance.


>+        static void performanceProfilerSetThreadName(char *name);

nit: that should be (const char* name)

[Sam Weinig]

What kind of profiler is this being used for?  Where will this be called in WebCore?

[Antony Sargent]

(In reply to comment #3)
> What kind of profiler is this being used for?  Where will this be called in
> WebCore?
> 

It's to expose control of Quantify (a companion tool to Purify, if you aren't familiar with it) to javascript functions in developer builds. For instance, imagine you have a layout test and want to profile some pieces of running javascript but ignore others; you can then write something like:

var profiler = new window.chromium.Profiler();
profiler.stop();
profiler.clear();
profiler.setThreadName("my test thread");
doSetupThatShouldntBeProfiled(); 
profiler.start();
doTest();
profiler.stop();

This is something that would only be exposed in developer builds compiled with Purify; not in regular builds. It's going in the Chromium bridge so that our javascript extension code can call through to the chrome code that links against the purify API. See these files for our abstraction of the Purify memory debugging API:

http://src.chromium.org/viewvc/chrome/trunk/src/base/memory_debug.h
http://src.chromium.org/viewvc/chrome/trunk/src/base/memory_debug.cc

(I'll be adding a similar class for the quantify performance profiler.) 

[Antony Sargent]

Created attachment 28110 [details]
Patch v2

[Darin Fisher (:fishd, Google)]

Comment on attachment 28110 [details]
Patch v2

>Index: platform/chromium/ChromiumBridge.h
...
>         // Widget -------------------------------------------------------------
>         static void widgetSetCursor(Widget*, const Cursor&);
>         static void widgetSetFocus(Widget*);
>+
>+        // Profiler -----------------------------------------------------------
>+        static void profilerStart();
>+        static void profilerStop();
>+        static void profilerClearData();
>+        static void profilerSetThreadName(const char *name);

one more stylistic problem that i forgot to mention... the groups in
ChromiumBridge are intentionally alphabetized, but this breaks that.
please sort them :)

also, please note that webkit style says to write |const char* name|
instead of |const char *name|.  sorry for not making that more clear
in my previous review.

otherwise, this change LG.  but please upload a clean patch.  thanks!

[Antony Sargent]

Created attachment 28179 [details]
Patch v3

[Darin Fisher (:fishd, Google)]

Antony, this bug can be closed now given your recent Chromium work, right?

[Antony Sargent]

Yes. 

(For the record and/or in case anyone else is interested, we found a better way to expose JS extensions in Chromium code that won't involve having to go through the ChromiumBridge. Those changes are at http://codereview.chromium.org/40132 )