Bug 74221 - Web Inspector: consider disabling network tracking while running the CPU profile
Summary: Web Inspector: consider disabling network tracking while running the CPU profile
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: Ilya Tikhonovsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-09 14:57 PST by Pavel Feldman
Modified: 2012-08-07 02:33 PDT (History)
14 users (show)

See Also:


Attachments
Patch (1.87 KB, patch)
2011-12-12 07:57 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (6.46 KB, patch)
2011-12-13 06:44 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (7.14 KB, patch)
2011-12-13 21:02 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (6.85 KB, patch)
2011-12-13 21:59 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (6.25 KB, patch)
2011-12-14 09:12 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-12-09 14:57:55 PST
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=====
Comment 1 Pavel Feldman 2011-12-09 15:30:41 PST
Same applies to the console logging (console.log(), etc.)
Comment 2 Ojan Vafai 2011-12-09 15:47:30 PST
Does the same apply to updating the Elements panel as well?
Comment 3 Pavel Feldman 2011-12-09 15:54:15 PST
(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.
Comment 4 Ilya Tikhonovsky 2011-12-12 07:57:23 PST
Created attachment 118788 [details]
Patch
Comment 5 Pavel Feldman 2011-12-12 08:41:04 PST
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.
Comment 6 Ilya Tikhonovsky 2011-12-13 06:44:32 PST
Created attachment 119010 [details]
Patch
Comment 7 WebKit Review Bot 2011-12-13 07:20:14 PST
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
Comment 8 Pavel Feldman 2011-12-13 13:19:04 PST
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.
Comment 9 Ilya Tikhonovsky 2011-12-13 21:02:15 PST
Created attachment 119151 [details]
Patch
Comment 10 Ilya Tikhonovsky 2011-12-13 21:59:09 PST
Created attachment 119153 [details]
Patch
Comment 11 Pavel Feldman 2011-12-14 01:37:41 PST
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.
Comment 12 Ilya Tikhonovsky 2011-12-14 04:23:00 PST
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.
Comment 13 Ilya Tikhonovsky 2011-12-14 04:42:40 PST
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.
Comment 14 Pavel Feldman 2011-12-14 08:39:35 PST
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.
Comment 15 Ilya Tikhonovsky 2011-12-14 09:12:07 PST
Created attachment 119231 [details]
Patch
Comment 16 Pavel Feldman 2011-12-14 09:21:47 PST
Comment on attachment 119231 [details]
Patch

Please remove the guards from enable and disable and it is good to go.
Comment 17 Pavel Feldman 2011-12-14 09:23:55 PST
Please remove the guards from enable and disable and it is good to go.
Comment 18 Ilya Tikhonovsky 2011-12-14 11:34:51 PST
Committed r102803
Comment 19 Pavel Feldman 2011-12-14 15:18:57 PST
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.
Comment 20 Vsevolod Vlasov 2012-08-07 02:33:31 PDT
 Chromium issue: http://code.google.com/p/chromium/issues/detail?id=107138