Summary: | Web Inspector: consider disabling network tracking while running the CPU profile | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pavel Feldman <pfeldman> | ||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Ilya Tikhonovsky <loislo> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, dglazkov, joepeck, keishi, loislo, ojan, pfeldman, pmuellr, rik, timothy, vsevik, webkit.review.bot, yurys | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Pavel Feldman
2011-12-09 14:57:55 PST
Same applies to the console logging (console.log(), etc.) Does the same apply to updating the Elements panel as well? (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. Created attachment 118788 [details]
Patch
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. Created attachment 119010 [details]
Patch
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 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. Created attachment 119151 [details]
Patch
Created attachment 119153 [details]
Patch
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 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 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 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. Created attachment 119231 [details]
Patch
Comment on attachment 119231 [details]
Patch
Please remove the guards from enable and disable and it is good to go.
Please remove the guards from enable and disable and it is good to go. Committed r102803 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. Chromium issue: http://code.google.com/p/chromium/issues/detail?id=107138 |