Bug 74221 – Web Inspector: consider disabling network tracking while running the CPU profile

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

[Pavel Feldman]

User report
=======8<=====
I've noticed that in some cases my profiles are thrown way off by really really slow calls to xhr.send. Profiling inside maps.google.com the xhr.send calls take up to 40ms!
I've learned that I can work around this by starting a profile at the console (console.profile()), closing dev tools, doing my test, then reopening dev tools and doing console.profileEnd(). It works but it's a big pain.
=======>8=====

[Pavel Feldman]

Same applies to the console logging (console.log(), etc.)

[Ojan Vafai]

Does the same apply to updating the Elements panel as well?

[Pavel Feldman]

(In reply to comment #2)
> Does the same apply to updating the Elements panel as well?

It probably does. We would also need to re-fetch the DOM tree and cached messages upon the profile completion.

[Ilya Tikhonovsky]

Created attachment 118788 [details]
Patch

[Pavel Feldman]

Comment on attachment 118788 [details]
Patch

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

> Source/WebCore/inspector/front-end/ProfileView.js:594
> +            NetworkAgent.enable();

You should adjust the code so that after enabling the agent ResourceTreeModel was going through _frontendReused.

[Ilya Tikhonovsky]

Created attachment 119010 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119010 [details]
Patch

Attachment 119010 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10847375

New failing tests:
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector-enabled/database-open.html

[Pavel Feldman]

Comment on attachment 119010 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        User report:

ChangeLog should not include all the bug report details. It should rather be a summary of the changes performed.

> Source/WebCore/inspector/front-end/NetworkManager.js:42
> +    this._resourceTrackingRequestSent = false;

this._resourceTracking should be sufficient.

[Ilya Tikhonovsky]

Created attachment 119151 [details]
Patch

[Ilya Tikhonovsky]

Created attachment 119153 [details]
Patch

[Pavel Feldman]

Comment on attachment 119153 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        As the result the CPU profiler's stats data are far away from the reality.

Not that far, just skewed.

> Source/WebCore/inspector/front-end/NetworkManager.js:86
> +                return;

Can this happen?

> Source/WebCore/inspector/front-end/NetworkManager.js:102
> +                return;

ditto

> Source/WebCore/inspector/front-end/ProfileView.js:591
>              ProfilerAgent.start();

You should proceed with the profiling from within the callback.

> Source/WebCore/inspector/front-end/ProfileView.js:594
> +            WebInspector.networkManager.enableResourceTracking();

ditto

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:50
> +    networkManager.enableResourceTracking();

You already enable resource tracking from the network manager constructor.

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:51
> +    this._resourceTrackingWasDisabled = false;

There is probably no need to mirror the network manager state here.

[Ilya Tikhonovsky]

Comment on attachment 119153 [details]
Patch

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

>> Source/WebCore/inspector/front-end/NetworkManager.js:86
>> +                return;
> 
> Can this happen?

It is public method and due to asynchronous nature of our protocol there is a chance that somebody call enableResourceTracking twice.
I had a guard for that in previous patch but you against it.
This version will send two enable command to the backend but ResourceTrackingEnabled event will be fired only for the first one.
Without such the guard ResourceTreeModel or another consumer for these events will re-fetch tree twice.

>> Source/WebCore/inspector/front-end/ProfileView.js:591
>>              ProfilerAgent.start();
> 
> You should proceed with the profiling from within the callback.

It is not really necessary to call it from a callback. protocol guarantee us that our commands will be executed sequentially.

[Ilya Tikhonovsky]

Comment on attachment 119153 [details]
Patch

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

>> Source/WebCore/inspector/front-end/ResourceTreeModel.js:51
>> +    this._resourceTrackingWasDisabled = false;
> 
> There is probably no need to mirror the network manager state here.

otherwise we will fetch the resource tree twice at start-up.
This increases start-up time and introduces test failures.

[Pavel Feldman]

Comment on attachment 119153 [details]
Patch

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

>>> Source/WebCore/inspector/front-end/NetworkManager.js:86
>>> +                return;
>> 
>> Can this happen?
> 
> It is public method and due to asynchronous nature of our protocol there is a chance that somebody call enableResourceTracking twice.
> I had a guard for that in previous patch but you against it.
> This version will send two enable command to the backend but ResourceTrackingEnabled event will be fired only for the first one.
> Without such the guard ResourceTreeModel or another consumer for these events will re-fetch tree twice.

I don't think we should expect the call site to be so much wrong.

>>> Source/WebCore/inspector/front-end/ProfileView.js:591
>>>              ProfilerAgent.start();
>> 
>> You should proceed with the profiling from within the callback.
> 
> It is not really necessary to call it from a callback. protocol guarantee us that our commands will be executed sequentially.

Right, but that's the only way to guarantee that by the time you issue ProfilerAgent.start(), this._resourceTracking in the network agent is "false".

>>> Source/WebCore/inspector/front-end/ResourceTreeModel.js:51
>>> +    this._resourceTrackingWasDisabled = false;
>> 
>> There is probably no need to mirror the network manager state here.
> 
> otherwise we will fetch the resource tree twice at start-up.
> This increases start-up time and introduces test failures.

Well, you could do NetworkAgent.enable() in the NetworkManager constructor to mitigate this. I.e. make "enabled" state default and then track changes from this enabled state.

[Ilya Tikhonovsky]

Created attachment 119231 [details]
Patch

[Pavel Feldman]

Comment on attachment 119231 [details]
Patch

Please remove the guards from enable and disable and it is good to go.

[Pavel Feldman]

Please remove the guards from enable and disable and it is good to go.

[Ilya Tikhonovsky]

Committed r102803

[Pavel Feldman]

Please close the bugs upon landing patches manually. Also, it sounds like the state flag in the NetworkManager is no longer used. Please remove it as a follow up to this change.

[Vsevolod Vlasov]

 Chromium issue: http://code.google.com/p/chromium/issues/detail?id=107138