Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
121653002:
Track AsyncReadback time using histogram. (Closed)
|
Created:
6 years, 12 months ago by sivag Modified:
6 years, 11 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, miu+watch_chromium.org, Ilya Sherman, Alexei Svitkine (slow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAs the AsyncReadBack is a hardware based approach, the performance can vary on different hardwares. Its worth tracking on Android.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246435
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changes done as per the review comments. #Patch Set 3 : Rebased TOT. #
Messages
Total messages: 18 (0 generated)
If needed we can add support to Aura as well. PTAL..
Hi Please take a look and provide your comments.
+jar for adding UMA stats I think something else needs to be updated for this to work for Chrome and the event naming might not match the conventions. Also, if you really want to measure performance you might want to measure in a different place (probably on the gpu thread). What this measures is the latency of reading back the contents on the UI thread. However, minimizing that latency is not necessarily a primary optimization goal. The code involved here has been written to minimize the time it blocks the UI (hence async with query callback) and also GPU thread (see PBO readback path with fences).
For the update thumbnail, this might not be a big concern, but for any features requesting screenshots, i wanted to make sure if there is too much delay when the functionality fails to provide the bitmap in time. This measures the turnaround time of copyfromcompositingsurface in app's point of view. Tracking the GPU readback ,is also a good option to know the hardware based perforamnce. I will look into this as well.
Hi Jar, Can you take a look at this patch?
Please also include edits to: tools/metrics/histograms/histograms.xml to provide the prose description for your histograms. In general, histogram results are gathered in Chrome, and only visible inside Google. Do you have a connection to Google that would help you harvest this data in some anonymized fashion that could help you? Lastly... I don't have OWNER rights to these directories, so you'll need another reviewer as well. https://codereview.chromium.org/121653002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/121653002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_android.cc:99: UMA_HISTOGRAM_TIMES("RWHVA.CopyFromCompositingSurface asyncreadback time", nit: Did you intend to add data to the same histogram in line 1401? If so, it would be nicer to only list this long name once, such as in a small static function (or in an anonymous namespace local to this file). Also, for naming conventions for histograms: What made you choose the prefix "RWHVA"? Although not illegal, we usually don't have spaces in the histogram name (and it is even stranger to mix BouncyCaps, combinednames, and spaces). I'd suggest something like: "Compositing.CopyFromSurfaceTime" for the above histogram, and maybe for line 611: "Compositing.CopyFromSurfaceTimeSynchronous" https://codereview.chromium.org/121653002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_android.cc:632: nit: no need for blank line
On 2014/01/11 03:41:38, jar wrote: > Please also include edits to: > > tools/metrics/histograms/histograms.xml > > to provide the prose description for your histograms. > > In general, histogram results are gathered in Chrome, and only visible inside > Google. Do you have a connection to Google that would help you harvest this > data in some anonymized fashion that could help you? > > Lastly... I don't have OWNER rights to these directories, so you'll need another > reviewer as well. > I can stamp once you are happy with it.
I am trying to measure the performance of the async readback here, from apps point of view as this can vary with hardware. I wanted to measure this data using chrome://histograms when needed. PTAL... https://codereview.chromium.org/121653002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/121653002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_android.cc:99: UMA_HISTOGRAM_TIMES("RWHVA.CopyFromCompositingSurface asyncreadback time", On 2014/01/11 03:41:39, jar wrote: > nit: Did you intend to add data to the same histogram in line 1401? If so, it > would be nicer to only list this long name once, such as in a small static > function (or in an anonymous namespace local to this file). > > Also, for naming conventions for histograms: > > What made you choose the prefix "RWHVA"? > > Although not illegal, we usually don't have spaces in the histogram name (and it > is even stranger to mix BouncyCaps, combinednames, and spaces). > > I'd suggest something like: > > "Compositing.CopyFromSurfaceTime" > > for the above histogram, and maybe for line 611: > > "Compositing.CopyFromSurfaceTimeSynchronous" > Done. https://codereview.chromium.org/121653002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_android.cc:632: On 2014/01/11 03:41:39, jar wrote: > nit: no need for blank line Done.
On 2014/01/11 03:41:38, jar wrote: > > In general, histogram results are gathered in Chrome, and only visible inside > Google. Do you have a connection to Google that would help you harvest this > data in some anonymized fashion that could help you? I didn't see an answer to the above question.
On 2014/01/15 03:09:08, jar wrote: > On 2014/01/11 03:41:38, jar wrote: > > > > In general, histogram results are gathered in Chrome, and only visible inside > > Google. Do you have a connection to Google that would help you harvest this > > data in some anonymized fashion that could help you? > > I didn't see an answer to the above question. Hi Jar, I am contributing to chrome for improving the asyncreadback support(crrev.com/88033002). I dont have any connection to google. I understand that histogram information is more useful for collecting statistics by taking feedback, my only intension is to check this data on various devices when tested.As this is a hardware dependent approach i believe collecting the data would be helpfull.For few of the ports the snapshot histograms are already in place. Thanks, Siva
On 2014/01/15 05:02:40, Sikugu wrote: > On 2014/01/15 03:09:08, jar wrote: > > On 2014/01/11 03:41:38, jar wrote: > > > > > > In general, histogram results are gathered in Chrome, and only visible > inside > > > Google. Do you have a connection to Google that would help you harvest this > > > data in some anonymized fashion that could help you? > > > > I didn't see an answer to the above question. > > Hi Jar, > I am contributing to chrome for improving the asyncreadback > support(crrev.com/88033002). I dont have any connection to google. > I understand that histogram information is more useful for collecting statistics > by taking feedback, > my only intension is to check this data on various devices when tested.As this > is a hardware dependent approach > i believe collecting the data would be helpfull.For few of the ports the > snapshot histograms are already in place. > > Thanks, > Siva So is the real goal to be able to visit: about:histograms and see data for a running client? ... rather than upload to Google?
On 2014/01/16 08:11:33, jar wrote: > On 2014/01/15 05:02:40, Sikugu wrote: > > On 2014/01/15 03:09:08, jar wrote: > > > On 2014/01/11 03:41:38, jar wrote: > > > > > > > > In general, histogram results are gathered in Chrome, and only visible > > inside > > > > Google. Do you have a connection to Google that would help you harvest > this > > > > data in some anonymized fashion that could help you? > > > > > > I didn't see an answer to the above question. > > > > Hi Jar, > > I am contributing to chrome for improving the asyncreadback > > support(crrev.com/88033002). I dont have any connection to google. > > I understand that histogram information is more useful for collecting > statistics > > by taking feedback, > > my only intension is to check this data on various devices when tested.As this > > is a hardware dependent approach > > i believe collecting the data would be helpfull.For few of the ports the > > snapshot histograms are already in place. > > > > Thanks, > > Siva > > So is the real goal to be able to visit: > about:histograms > and see data for a running client? ... rather than upload to Google? Yes this is the idea. I thought this is acceptable as other snapshot histograms are already in place.
On 2014/01/16 08:11:33, jar wrote: > On 2014/01/15 05:02:40, Sikugu wrote: > > On 2014/01/15 03:09:08, jar wrote: > > > On 2014/01/11 03:41:38, jar wrote: > > > > > > > > In general, histogram results are gathered in Chrome, and only visible > > inside > > > > Google. Do you have a connection to Google that would help you harvest > this > > > > data in some anonymized fashion that could help you? > > > > > > I didn't see an answer to the above question. > > > > Hi Jar, > > I am contributing to chrome for improving the asyncreadback > > support(crrev.com/88033002). I dont have any connection to google. > > I understand that histogram information is more useful for collecting > statistics > > by taking feedback, > > my only intension is to check this data on various devices when tested.As this > > is a hardware dependent approach > > i believe collecting the data would be helpfull.For few of the ports the > > snapshot histograms are already in place. > > > > Thanks, > > Siva > > So is the real goal to be able to visit: > about:histograms > and see data for a running client? ... rather than upload to Google? Hi Jar, Any update on this review.?
LGTM
On 2014/01/13 20:59:06, sievers wrote: > On 2014/01/11 03:41:38, jar wrote: > > Please also include edits to: > > > > tools/metrics/histograms/histograms.xml > > > > to provide the prose description for your histograms. > > > > In general, histogram results are gathered in Chrome, and only visible inside > > Google. Do you have a connection to Google that would help you harvest this > > data in some anonymized fashion that could help you? > > > > Lastly... I don't have OWNER rights to these directories, so you'll need > another > > reviewer as well. > > > > I can stamp once you are happy with it. Hi Sievers, Please take a look at the changes.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/121653002/200001
Message was sent while issue was closed.
Change committed as 246435 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
