Bug 100103 – Web Inspector: Refactor Console's profiling feature to use InspectorProfilerAgent

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

[Vivek Galatage]

Refactor Console's profiling feature to use InspectorProfilerAgent

[Vivek Galatage]

Created attachment 170115 [details]
Patch

[WebKit Review Bot]

Attachment 170115 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/inspector/InspectorInstrumentation.h:216:  The parameter name "state" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorInstrumentation.h:216:  The parameter name "callStack" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorInstrumentation.h:217:  The parameter name "state" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorInstrumentation.h:217:  The parameter name "callStack" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorInstrumentation.h:403:  The parameter name "state" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorInstrumentation.h:403:  The parameter name "callStack" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorInstrumentation.h:404:  The parameter name "state" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorInstrumentation.h:404:  The parameter name "callStack" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:85:  The parameter name "state" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:85:  The parameter name "callStack" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:87:  The parameter name "state" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:87:  The parameter name "callStack" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:111:  The parameter name "state" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 13 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vivek Galatage]

Created attachment 170117 [details]
Patch

[Build Bot]

Comment on attachment 170117 [details]
Patch

Attachment 170117 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14518212

New failing tests:
fast/profiler/document-dot-write.html
fast/profiler/nested-anonymous-functon.html
fast/profiler/simple-no-level-change.html
fast/profiler/anonymous-function-calls-built-in-functions.html
fast/profiler/multiple-frames.html
fast/profiler/apply.html
fast/profiler/constructor.html
fast/profiler/built-in-function-calls-user-defined-function.html
fast/profiler/execution-context-and-eval-on-same-line.html
fast/profiler/nested-start-and-stop-profiler.html
fast/profiler/multiple-and-different-scoped-anonymous-function-calls.html
fast/profiler/profiling-from-a-nested-location.html
fast/profiler/multiple-and-different-scoped-function-calls.html
fast/profiler/one-execution-context.html
fast/profiler/simple-event-call.html
fast/profiler/multiple-anonymous-functions-called-from-the-same-function.html
fast/profiler/named-functions-with-display-names.html
fast/profiler/anonymous-event-handler.html
fast/profiler/inline-event-handler.html
fast/profiler/built-in-function-calls-anonymous.html
fast/profiler/profiling-from-a-nested-location-but-stop-profiling-outside-the-nesting.html
fast/profiler/anonymous-functions-with-display-names.html
fast/profiler/anonymous-function-calls-eval.html
fast/profiler/call.html
fast/profiler/many-calls-in-the-same-scope.html
fast/profiler/calling-the-function-that-started-the-profiler-from-another-scope.html
fast/loader/javascript-url-in-object.html
fast/profiler/anonymous-function-called-from-different-contexts.html
fast/profiler/event-handler.html
fast/profiler/profile-calls-in-included-file.html

[Ilya Tikhonovsky]

Comment on attachment 170117 [details]
Patch

Why do you need this?
InspectorController creates InspectorProfilerAgent in constructor.
Thus there is no profiler agent instance if WebInspector hasn't been opened. r-

[Ilya Tikhonovsky]

Comment on attachment 170117 [details]
Patch

looks like I was wrong. ProfilerAgent is always exist and enabled. return r?

[Pavel Feldman]

Comment on attachment 170117 [details]
Patch

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

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:438
> +        int resolvedLineNumber = 0;

Looks like copy-paste to me. Can we avoid it?

> Source/WebCore/inspector/InspectorProfilerAgent.h:85
> +    virtual void initiateProfiling(ErrorString* = 0, const String* title = 0, ScriptState* = 0, ScriptCallStack* = 0);

I would call them consoleProfile and consoleProfileEnd for clarity. They don't belong to the protocol, so they should not have ErrorString and should be in a separate group. We also don't pass strings by pointer.

> Source/WebCore/page/Console.cpp:275
> +    InspectorInstrumentation::startProfiling(page, title, state, callStack.get());

I would call them consoleProfile and consoleProfileEnd for clarity.

[Vivek Galatage]

Comment on attachment 170117 [details]
Patch

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

>> Source/WebCore/inspector/InspectorProfilerAgent.cpp:438
>> +        int resolvedLineNumber = 0;
> 
> Looks like copy-paste to me. Can we avoid it?

I am not sure if we can avoid this as these variables are locally used. Maybe an inline function is what are you suggesting?

>> Source/WebCore/inspector/InspectorProfilerAgent.h:85
>> +    virtual void initiateProfiling(ErrorString* = 0, const String* title = 0, ScriptState* = 0, ScriptCallStack* = 0);
> 
> I would call them consoleProfile and consoleProfileEnd for clarity. They don't belong to the protocol, so they should not have ErrorString and should be in a separate group. We also don't pass strings by pointer.

Sure I will make these changes to the naming of the functions. 

The reason I am using the String pointer is because the argument title for stop() in the InspectorProfilerAgent is optional whereas the console::profileEnd though specified as optional, ends up sending NULL string. So now to differentiate the call site, I thought of using the pointer.

>> Source/WebCore/page/Console.cpp:275
>> +    InspectorInstrumentation::startProfiling(page, title, state, callStack.get());
> 
> I would call them consoleProfile and consoleProfileEnd for clarity.

Sure will modify accordingly.

[Vivek Galatage]

Created attachment 171383 [details]
Patch

[Pavel Feldman]

Comment on attachment 171383 [details]
Patch

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

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:83
> +        if (state)

I don't think you should merge these into startProfiling just to fork them immediately based on the state value.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:383
> +void InspectorProfilerAgent::consoleProfile(const String* title, ScriptState* state, ScriptCallStack* callStack)

Overall, I find this rather a step back from the original design. You merge two code flows (user initiated profile taking and console.profile) to go through one method and then distinguish between them based on the status / title presence. The original idea was that there is a "profiler backend" implemented in ScriptProfiler that could be used by both: profiler agent and the console independently. But then we needed to make them mutually exclusive and re-used the profile name sequence, hence the existing instrumentation methods. I agree that this change makes some thing better, but at the same time it somewhat obfuscates the console-based profiler taking that is currently very readable.

What was the original motivation behind the change? Maybe we should move the getCurrentUserInitiatedProfileName into ScriptProfiler backend instead and entirely split console and agent code paths?

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:400
> +    startProfiling(resolvedTitle, state);

I.e. just inline ScriptProfiler::start(state, title) here.

[Vivek Galatage]

(In reply to comment #10)

Thank you Pavel for your feedback.

> (From update of attachment 171383 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171383&action=review
> 
> > Source/WebCore/inspector/InspectorProfilerAgent.cpp:83
> > +        if (state)
> 
> I don't think you should merge these into startProfiling just to fork them immediately based on the state value.

Yeah I also felt it the same after submitting this patch :) May be its better the way currently its there and its much more readable and the paths are very clear. I messed it here and its not an elegant way of handling it either.
> 
> > Source/WebCore/inspector/InspectorProfilerAgent.cpp:383
> > +void InspectorProfilerAgent::consoleProfile(const String* title, ScriptState* state, ScriptCallStack* callStack)
> 
> Overall, I find this rather a step back from the original design. You merge two code flows (user initiated profile taking and console.profile) to go through one method and then distinguish between them based on the status / title presence. The original idea was that there is a "profiler backend" implemented in ScriptProfiler that could be used by both: profiler agent and the console independently. But then we needed to make them mutually exclusive and re-used the profile name sequence, hence the existing instrumentation methods. I agree that this change makes some thing better, but at the same time it somewhat obfuscates the console-based profiler taking that is currently very readable.
> 
> What was the original motivation behind the change? Maybe we should move the getCurrentUserInitiatedProfileName into ScriptProfiler backend instead and entirely split console and agent code paths?
> 

The motivation was to confine the code paths to one place so that it becomes easier to maintain. e.g. there exist some bugs #21259, #19947 etc. So while digging through the code base, I thought it would be better if these things can be managed at one place. So once this re-factoring was over, I thought of fixing these other bugs. Also the current profiles panel is little buggy. In that if you start a JS CPU profile and switch to CSS or memory heap while the first one is ongoing, the front-end still shows the button states in recording. Whereas if you click the button, it actually starts recording the new profile type. So with all these things around, I thought of clearing things one after another. But may be I am wrong with this. 

Moving common things back into the Profiler back-end sounds good and more elegant than the currently what I proposed. Also if required we can separate these code paths completely. But cases like reported in bug #19947 should they be handled - I mean should we show the ongoing profiles started by using console.profile in the profiles panel?  These are shown as of today once we use console.profileEnd().

[Pavel Feldman]

> Also the current profiles panel is little buggy. In that if you start a JS CPU profile and switch to CSS or memory heap while the first one is ongoing, the front-end still shows the button states in recording. Whereas if you click the button, it actually starts recording the new profile type. So with all these things around, I thought of clearing things one after another. But may be I am wrong with this. 

This is a known thing, that's why we are working on re-implementing Profiles landing page as a whole.

Wrt Bug 19947, I now see why you wanted to see the code in one common place. Yet, there is enough difference between user-initiated profiles and console.profile-based ones. For example, we don't add console message for manual profiles and after your change we will. How about you simply move profile-related methods into the InspectorProfilerAgent for now and only reuse what is obviously repeated, while leaving the user-initialted and console-initiated code paths separate?

[Vivek Galatage]

(In reply to comment #12)
> > Also the current profiles panel is little buggy. In that if you start a JS CPU profile and switch to CSS or memory heap while the first one is ongoing, the front-end still shows the button states in recording. Whereas if you click the button, it actually starts recording the new profile type. So with all these things around, I thought of clearing things one after another. But may be I am wrong with this. 
> 
> This is a known thing, that's why we are working on re-implementing Profiles landing page as a whole.
> 
> Wrt Bug 19947, I now see why you wanted to see the code in one common place. Yet, there is enough difference between user-initiated profiles and console.profile-based ones. For example, we don't add console message for manual profiles and after your change we will. 

Even I have seen messages like 'Profile "Profile n" started/finished' messages for the manual profiles. I think by manual you mean the profiles started using the profiles panel record button. I am seeing this in Chrome canary Version 24.0.1312.0 canary. So is this desired a behavior or something has changed here as well? 

Also the links like "webkit-profile://CPU/org.webkit.profiles.user-initiated.1#2" shown on console are also not taking to the profiles panel. May be this is a regression as it was working earlier. Should we file this as another bug for tracking purpose. (I know its not a priority item though)

> How about you simply move profile-related methods into the InspectorProfilerAgent for now and only reuse what is obviously repeated, while leaving the user-initialted and console-initiated code paths separate?

Yeah it sounds good to me. But the code is not really duplicated wrt current implementation. So I am not sure how much of code re-use we would be able to achieve with this. Moving the profile number generation if it wasn't provided inside the ScriptProfiler would do some good though. Your thoughts?

[Pavel Feldman]

> Even I have seen messages like 'Profile "Profile n" started/finished' messages for the manual profiles. I think by manual you mean the profiles started using the profiles panel record button. I am seeing this in Chrome canary Version 24.0.1312.0 canary. So is this desired a behavior or something has changed here as well? 

Sounds like a regression.

> Also the links like "webkit-profile://CPU/org.webkit.profiles.user-initiated.1#2" shown on console are also not taking to the profiles panel. May be this is a regression as it was working earlier. Should we file this as another bug for tracking purpose. (I know its not a priority item though)

Sounds like a regression as well (probably due to the custom scheme handling code).

> Yeah it sounds good to me. But the code is not really duplicated wrt current implementation. So I am not sure how much of code re-use we would be able to achieve with this. Moving the profile number generation if it wasn't provided inside the ScriptProfiler would do some good though. Your thoughts?

I think if you start with simply moving the code, it will do no harm and will allow further refactorings. Like you would be able to uniformly pass profiler state to the front-end on both actions in order to fix the Bug 19947 if necessary.

[Vivek Galatage]

(In reply to comment #14)
> I think if you start with simply moving the code, it will do no harm and will allow further refactorings. Like you would be able to uniformly pass profiler state to the front-end on both actions in order to fix the Bug 19947 if necessary.

Sure, I will do that. Thank you for your quick feedback. :)

[Pavel Feldman]

Comment on attachment 171383 [details]
Patch

Clearing r? as per discussion in comments #14, #15