Last Comment Bug 772665 - Add console.profile API
: Add console.profile API
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on: 773428
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-10 15:22 PDT by Benoit Girard (:BenWa)
Modified: 2012-07-13 05:31 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Example (View in webkit with 'Profiles' devtool) (257 bytes, text/html)
2012-07-10 15:23 PDT, Benoit Girard (:BenWa)
no flags Details
Part 1: Expose console.profile{End}(name) (4.42 KB, patch)
2012-07-10 22:09 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 1: Expose console.profile{End}(name) (2.71 KB, patch)
2012-07-10 22:09 PDT, Benoit Girard (:BenWa)
rcampbell: review+
Details | Diff | Splinter Review
Add console.profile to test_consoleAPI.html (1016 bytes, patch)
2012-07-12 14:29 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-07-10 15:22:00 PDT
Webkit already has console.profile{End}(name). It instruments each JS function. I believe the API only works if you have the 'Profiles' pane of their tools open. It would be nice for web developers if we supported the same API. Here are my concerns:

- Can we just implement their API?
- Currently our profiler is design differently:
 (1) We can't nested profiling session, although we may be able to use markers to simulate having multiple profiling session. We could also just fix this limitation.
 (2) We have a circular buffer so we will drop samples after a while. We could have an increasing buffer size or we can just make it clear in the UI that you've dropped samples.
- To compensate for these limitations we have the benefit of being able to expend into the platform code.
- We don't the profiler built in so either we only enable it with the extension only or we make moving the profiler as a devtools a pre-req.
Comment 2 Benoit Girard (:BenWa) 2012-07-10 15:23:59 PDT
Created attachment 640819 [details]
Example (View in webkit with 'Profiles' devtool)
Comment 3 :Ehsan Akhgari 2012-07-10 17:19:58 PDT
(In reply to comment #0)
> - Can we just implement their API?

Absolutely!  :-)

> - Currently our profiler is design differently:
>  (1) We can't nested profiling session, although we may be able to use markers
> to simulate having multiple profiling session. We could also just fix this
> limitation.

I like simple solutions more, and I like us to start using just a marker and see what goes wrong.  I don't suspect anything will, but see below.

>  (2) We have a circular buffer so we will drop samples after a while. We could
> have an increasing buffer size or we can just make it clear in the UI that
> you've dropped samples.

We should look into how V8 stores their profiles.  We could do dynamic page allocation to be able to store arbitrary profile sizes, but I don't necessarily think that's a requirement for this bug.  For now we can just crop profiles when we rotate once over our buffer.

> - To compensate for these limitations we have the benefit of being able to
> expend into the platform code.

Right, but I'm not sure if that's something that we want to expose to web developers...

> - We don't the profiler built in so either we only enable it with the extension
> only or we make moving the profiler as a devtools a pre-req.

The console.profile API may just be a no-op if the profiler is not active.  We don't need to advertize its usage to web developers until we have a profiler UI in place.
Comment 4 :Ehsan Akhgari 2012-07-10 17:27:33 PDT
Rob, who's our console.* guru?
Comment 5 Benoit Girard (:BenWa) 2012-07-10 17:28:46 PDT
Can we find someone to own implementing console.profile? I already have quite a bit on my plate. 'tools/profiler/sampler.h' should expose the API that's needed to implement this so this should be easy.
Comment 6 :Ehsan Akhgari 2012-07-10 17:31:16 PDT
(In reply to comment #5)
> Can we find someone to own implementing console.profile? I already have quite a
> bit on my plate. 'tools/profiler/sampler.h' should expose the API that's needed
> to implement this so this should be easy.

See comment 4.  ;-)
Comment 7 Benoit Girard (:BenWa) 2012-07-10 19:35:04 PDT
I just took a look and the code is here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.js

I think I can handle it:
- I'm going to expose the calls to console
- The calls will fire an observable event
- The profile add-on will pick up the observable event and will either ignore them if the user doesn't have the feature enable or will honor the events and insert markers.
Comment 8 :Ehsan Akhgari 2012-07-10 20:24:59 PDT
(In reply to comment #7)
> I just took a look and the code is here:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.js
> 
> I think I can handle it:
> - I'm going to expose the calls to console
> - The calls will fire an observable event
> - The profile add-on will pick up the observable event and will either ignore
> them if the user doesn't have the feature enable or will honor the events and
> insert markers.

Sounds reasonable to me.
Comment 9 Benoit Girard (:BenWa) 2012-07-10 22:09:07 PDT
Created attachment 640917 [details] [diff] [review]
Part 1: Expose console.profile{End}(name)

See Comment 7 for my design.

This patch should have no impact for any configuration without the profiler addon. It should send events into nothing.
Comment 10 Benoit Girard (:BenWa) 2012-07-10 22:09:53 PDT
Created attachment 640918 [details] [diff] [review]
Part 1: Expose console.profile{End}(name)
Comment 11 Rob Campbell [:rc] (:robcee) 2012-07-12 12:57:55 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> Rob, who's our console.* guru?

that'd likely be mihai or, if pressed, myself. I'll ask mihai for a pass on this too.
Comment 12 Rob Campbell [:rc] (:robcee) 2012-07-12 12:58:37 PDT
Comment on attachment 640918 [details] [diff] [review]
Part 1: Expose console.profile{End}(name)

actually, we don't really need a mihai review on this. It's pretty simple.

Thanks BenWa!
Comment 13 Benoit Girard (:BenWa) 2012-07-12 13:00:38 PDT
W00t. I was waiting for this r+ to finish the extension changes. Once the extension is ready I'll push this.
Comment 14 Benoit Girard (:BenWa) 2012-07-12 13:06:04 PDT
Actually I changed my mind. Let's get this into a nightly ASAP. Right now 'Gecko Profiler' will simply echo the request and soon will handle it correctly.
Comment 16 Benoit Girard (:BenWa) 2012-07-12 13:49:15 PDT
I have a prototype working on the extension.

Here's a profile of the sample page above. We should get a more interesting testcase:
http://people.mozilla.com/~bgirard/cleopatra/?report=bc2d6ed3b470fe61afca6c41f76c77c68c9b3948
Comment 18 Benoit Girard (:BenWa) 2012-07-12 14:29:31 PDT
Created attachment 641603 [details] [diff] [review]
Add console.profile to test_consoleAPI.html

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