Bug 97347 – [chromium] Expose image decode and image scale time to WebViewImpl renderingStats

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

[Nick Carter]

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).

[Nick Carter]

Created attachment 166078 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[Adam Barth]

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.

[Nick Carter]

Created attachment 166090 [details]
Patch

[Adam Barth]

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.

[Pavel Feldman]

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?

[Nick Carter]

Created attachment 166338 [details]
Patch

[Nick Carter]

(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.

[Nick Carter]

Created attachment 166350 [details]
Patch

[Pavel Feldman]

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?

[Pavel Feldman]

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.

[Nick Carter]

(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.

[Nat Duca]

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.

[Pavel Feldman]

> 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.

[Timothy Hatcher]

Chromium and V8 have left the building. Won't fix.