Bug 74391 - Web Inspector: make ProfilesPanel scale as the number of ProfileTypes grows
Summary: Web Inspector: make ProfilesPanel scale as the number of ProfileTypes grows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks: 74004
  Show dependency treegraph
 
Reported: 2011-12-13 01:58 PST by Alexander Pavlov (apavlov)
Modified: 2011-12-15 01:37 PST (History)
11 users (show)

See Also:


Attachments
Patch (20.45 KB, patch)
2011-12-13 02:54 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (13.59 KB, patch)
2011-12-14 08:59 PST, Alexander Pavlov (apavlov)
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2011-12-13 01:58:22 PST
Patch to follow.
Comment 1 Alexander Pavlov (apavlov) 2011-12-13 02:54:25 PST
Created attachment 118987 [details]
Patch
Comment 2 Pavel Feldman 2011-12-13 03:28:06 PST
Comment on attachment 118987 [details]
Patch

Few things are broken here: there is no need to take concurrent profiles. And profiled agent should only do CPU profiling.
Comment 3 Alexander Pavlov (apavlov) 2011-12-13 03:31:27 PST
(In reply to comment #2)
> (From update of attachment 118987 [details])
> Few things are broken here: there is no need to take concurrent profiles. And profiled agent should only do CPU profiling.

Could you please be more verbose and suggest the desired approach? From our last discussion offline I gained an impression that the entire thing should be exposed through the Profiles panel, which quite logically interacts with the InspectorProfilerAgent.
Comment 4 Alexander Pavlov (apavlov) 2011-12-13 03:33:37 PST
(In reply to comment #2)
> (From update of attachment 118987 [details])
[snip]
> And profiled agent should only do CPU profiling.

As a side note, currently InspectorProfilerAgent deals not only with CPU profiling but also with heap snapshotting which is quite a different thing.
Comment 5 Pavel Feldman 2011-12-13 04:18:17 PST
> Could you please be more verbose and suggest the desired approach? From our last discussion offline I gained an impression that the entire thing should be exposed through the Profiles panel, which quite logically interacts with the InspectorProfilerAgent.

InspectorProfilerAgent is responsible for the JavaScript profiling domain. Instead of putting more things into it, we should extract heap profiling routines from it into a separate agent. So we do expose various profilings in the Profiles panel, but you should use different agents for that. In case of CSS profiling, it would make more sense to put corresponding backend routines into the CSS domain agent.

The other thing is that taking several profiles at a time is fundamentally wrong. Any background activity is skewing the profile. We are currently working on disabling unrelated agents while CPU profiling. Enabling concurrent profiles is moving us in the opposite direction.
Comment 6 Mikhail Naganov 2011-12-13 04:25:51 PST
(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 118987 [details] [details])
> [snip]
> > And profiled agent should only do CPU profiling.
> 
> As a side note, currently InspectorProfilerAgent deals not only with CPU profiling but also with heap snapshotting which is quite a different thing.

View in context: https://bugs.webkit.org/attachment.cgi?id=118987&action=review

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:310
> +        String title = getCurrentUserInitiatedProfileName(true);

That means, we will share m_currentUserInitiatedProfileNumber for all kinds of profiles. Are you OK with that?

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:340
> +        String title = getCurrentUserInitiatedProfileName();

Hmm. Will it be possible to have two profiles of different kinds started in parallel? If yes, you'll have a problem with a shared counter here.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:825
> +        return this["_temporaryRecordingProfile" + profileType];

Ouch. Perhaps, using a map object called "this._temporaryRecordingProfile" with entries for profileTypes will look better?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:840
>                      title: WebInspector.UIString("Recordingâ¦"),

Not your fault, but could I ask you to change the string to use a char code for ellipsis, please?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:862
>                      title: WebInspector.UIString("Snapshottingâ¦"),

And here as well.
Comment 7 Alexander Pavlov (apavlov) 2011-12-14 05:35:07 PST
(In reply to comment #5)
> > Could you please be more verbose and suggest the desired approach? From our last discussion offline I gained an impression that the entire thing should be exposed through the Profiles panel, which quite logically interacts with the InspectorProfilerAgent.
> 
> InspectorProfilerAgent is responsible for the JavaScript profiling domain. Instead of putting more things into it, we should extract heap profiling routines from it into a separate agent. So we do expose various profilings in the Profiles panel, but you should use different agents for that. In case of CSS profiling, it would make more sense to put corresponding backend routines into the CSS domain agent.

Got it, will do.

> The other thing is that taking several profiles at a time is fundamentally wrong. Any background activity is skewing the profile. We are currently working on disabling unrelated agents while CPU profiling. Enabling concurrent profiles is moving us in the opposite direction.

Good point. Do we have a thought-out approach that scales relative to the number of different profile types? Shall we forbid launching of other profile types?
Comment 8 Pavel Feldman 2011-12-14 08:48:51 PST
> Good point. Do we have a thought-out approach that scales relative to the number of different profile types? Shall we forbid launching of other profile types?

I think we should do a welcome page on the ProfilesPanel much like we have one for the Audits. That's the place where users will start various profiles (instead of the poorly discoverable icons in the status bar). Then we simply forbit taking a new profile while some profiling is running. What do you think?
Comment 9 Alexander Pavlov (apavlov) 2011-12-14 08:59:08 PST
Created attachment 119227 [details]
Patch
Comment 10 Pavel Feldman 2011-12-15 00:14:06 PST
Comment on attachment 119227 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119227&action=review

> Source/WebCore/inspector/front-end/ProfilesPanel.js:506
> +    updateProfileTypeButtons: function(isProfiling, effectButton)

Please annotate this using jsdoc.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:759
> +                this._profileTypeButtonsByIdMap[typeId].element.removeStyleClass("hidden");

this._profileTypeButtonsByIdMap[typeId].visible = true;

> Source/WebCore/inspector/front-end/ProfilesPanel.js:767
> +                this._profileTypeButtonsByIdMap[typeId].element.addStyleClass("hidden");

this._profileTypeButtonsByIdMap[typeId].visible = false;
Comment 11 Alexander Pavlov (apavlov) 2011-12-15 01:37:48 PST
Committed r102906: <http://trac.webkit.org/changeset/102906>