Bug 74391 – Web Inspector: make ProfilesPanel scale as the number of ProfileTypes grows

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 118987 [details]
Patch

[Pavel Feldman]

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.

[Alexander Pavlov (apavlov)]

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

[Alexander Pavlov (apavlov)]

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

[Pavel Feldman]

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

[Mikhail Naganov]

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

[Alexander Pavlov (apavlov)]

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

[Pavel Feldman]

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

[Alexander Pavlov (apavlov)]

Created attachment 119227 [details]
Patch

[Pavel Feldman]

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;

[Alexander Pavlov (apavlov)]

Committed r102906: <http://trac.webkit.org/changeset/102906>