WebRenderingStats should be extended with fields tracking time spent in the activities of image scaling and image decoding. WebViewImpl::renderingStats should fill in these new fields. Tactically, it makes sense to build on top of the instrumentation already in place in PlatformInstrumentation.h (e.g. PlatformInstrumentation::willDecodeImage).
Created attachment 166078 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Attachment 166078 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/Platform/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/Platform/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Source/Platform/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] Source/Platform/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] Source/Platform/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] Source/Platform/ChangeLog:13: Line contains tab character. [whitespace/tab] [5] Source/Platform/ChangeLog:14: Line contains tab character. [whitespace/tab] [5] Source/Platform/ChangeLog:15: Line contains tab character. [whitespace/tab] [5] Source/Platform/ChangeLog:16: Line contains tab character. [whitespace/tab] [5] Total errors found: 9 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 166078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166078&action=review > Source/WebKit/chromium/public/WebView.h:-117 > - Please don't remove these blank lines. They're here for a reason. > Source/WebKit/chromium/public/WebView.h:472 > + virtual void enableRenderingStatsCollection(bool enable) { } > + virtual bool isRenderingStatsCollectionEnabled() { return false; } Should this be a WebSettings instead? > Source/WebKit/chromium/public/WebView.h:473 > You removed the extra blank line we have between sections here.
Created attachment 166090 [details] Patch
Comment on attachment 166090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166090&action=review > Source/WebKit/chromium/public/WebView.h:-117 > - You didn't address my whitespace comments on the previous patch. :) > Source/WebKit/chromium/public/WebView.h:472 > + virtual void enableRenderingStatsCollection(bool enable) { } > + virtual bool isRenderingStatsCollectionEnabled() { return false; } You also didn't address my comments regarding these functions.
Why do you want this information to be exposed into WebKit? It sounds like TRACE_EVENT is already doing that. Why do we want yet another instrumentation signal for the embedder?
Created attachment 166338 [details] Patch
(In reply to comment #7) > Why do you want this information to be exposed into WebKit? It sounds like TRACE_EVENT is already doing that. Why do we want yet another instrumentation signal for the embedder? The goal is to get these stats so that they are visible to the gpu benchmarking extension, which is set up for regular, automated benchmarking. It is true, there is a lot of overlap between the three tracing systems: WebRenderingStats/CCRenderingStats, TRACE_EVENT, and the inspector tracing. I think that criticism is valid, but is it applicable to this patch? If you see an better way to use existing collectors, or to route the collected data to GpuBenchmarkingWrapper::GetRenderingStats, please let me know. TRACE_EVENT seems to work globally, and doesn't credit things back to a particular page. The Inspector framework is strongly tied to the idea of the inspector UI; while I've reused a low level part of that (PlatformInstrumentation), I did consider hooking into the InspectorController somehow -- that would have required redefining things so that you could have an active InstrumentingAgent with no inspector front-ends running. That means changing an assumption that seemed pretty baked-in to the code.
Created attachment 166350 [details] Patch
Automated benchmarking should be done using remote debugging protocol. It works for all platforms via web sockets, it is exposed via the chrome.debugger api in chrome. You can already get that information there. Inspector uses client-server approach and there are numerous headless clients. Are you in sync with Nat?
Before filing subsequent patches, could you explain intended mode of operation in greater detail? I think that existing model with TRACE_EVENT and inspector instrumentation introduces a lot of confusion already. I'd much rather invest into fixing it than introduce yet another reporting channel.
(In reply to comment #12) > Before filing subsequent patches, could you explain intended mode of operation in greater detail? I think that existing model with TRACE_EVENT and inspector instrumentation introduces a lot of confusion already. I'd much rather invest into fixing it than introduce yet another reporting channel. I don't see that I'm adding a new "mode of operation" at all, so it's tough for me to answer your question. I see this patch as extending the existing renderingStats(WebRenderingStats&) mode of operation. Are you saying that renderingStats() should not exist? (In reply to comment #11) > Automated benchmarking should be done using remote debugging protocol. It works for all platforms via web sockets, it is exposed via the chrome.debugger api in chrome. You can already get that information there. Inspector uses client-server approach and there are numerous headless clients. Are you in sync with Nat? Yes, Nat requested this patch. I've asked him to chime in here. Meanwhile, I'll have a look at your suggestion of using chrome.debugger from the gpu benchmarking extension to get these and other stats. If it does work, and the instrumentation overhead is acceptable, it sounds great.
The correct architectural way to get performance information up to a benchmark is through the devtools api, I agree. The thing that bugs me is enabling the entire tracing system just to get the time spent in image decoding from the test start to test finish. I know timeline is low-ish overhead, but in this case, it feels like the most efficient thing to do for a benchmark is to have an agent that tracks this time and just let the test retrieve the totals after the fact. Have you guys ever thought about a Counters agent? E.g. "Counters.enable", "Counters.get"? Would be useful for exposing a lot of stuff that we currently expose with a backdoor, e.g. heap size, gpu memory space, etc.
> Have you guys ever thought about a Counters agent? E.g. "Counters.enable", "Counters.get"? Would be useful for exposing a lot of stuff that we currently expose with a backdoor, e.g. heap size, gpu memory space, etc. Sorry, thread got lost in the email. Yes, we were. We'd like to have generic counters / histograms infrastructure. I think Jochen wanted it for his page cyclers as well. It is just that we don't have it scheduled for implementation yet.
Chromium and V8 have left the building. Won't fix.