Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10787025:
Expose a gpu-benchmark-only measureRecordTime API to JS. This function measures the time it takes t… (Closed)
|
Created:
8 years, 5 months ago by alokp Modified:
8 years, 4 months ago CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, jam, apatrick_chromium, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionExpose a gpu-benchmark-only printToSkPicture API to JS. This function records the page to an SkPicture, and dumps the resulting picture to a local file.
BUG=130422
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149287
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Total comments: 8
Patch Set 6 : #
Total comments: 1
Patch Set 7 : #Patch Set 8 : #
Total comments: 24
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Messages
Total messages: 27 (0 generated)
Not ready for checking in. Just wanted to give you an update on the direction I am heading. Please let me know of any issues/suggestions.
Cool http://codereview.chromium.org/10787025/diff/1/content/renderer/gpu/gpu_bench... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/1/content/renderer/gpu/gpu_bench... content/renderer/gpu/gpu_benchmarking_extension.cc:38: "chrome.gpuBenchmarking.measureRecordTime = function(dump) {" How about dumpToSkPicture = function(filename) http://codereview.chromium.org/10787025/diff/1/content/renderer/gpu/gpu_bench... content/renderer/gpu/gpu_benchmarking_extension.cc:106: if (dump) { I'm not sure you need to measure recording time. You're jut going to piggy back on the measuring system since thats what we happen to have called the method... Maybe we should rename that method to be paint() http://codereview.chromium.org/10787025/diff/1/content/renderer/gpu/gpu_bench... content/renderer/gpu/gpu_benchmarking_extension.cc:108: // resulting SkPicture will be dumped. Use the filename that the script provides. The automation harness (or the user) can put in the filename themselves, rather than having Chrome prompt
http://codereview.chromium.org/10787025/diff/1/content/renderer/gpu/gpu_bench... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/1/content/renderer/gpu/gpu_bench... content/renderer/gpu/gpu_benchmarking_extension.cc:38: "chrome.gpuBenchmarking.measureRecordTime = function(dump) {" On 2012/07/16 22:28:40, nduca wrote: > How about > > dumpToSkPicture = function(filename) > I chose this name because it seemed more appropriate for a class named Benchmarking. dump/printToSkPicture is OK too. http://codereview.chromium.org/10787025/diff/1/content/renderer/gpu/gpu_bench... content/renderer/gpu/gpu_benchmarking_extension.cc:108: // resulting SkPicture will be dumped. On 2012/07/16 22:28:40, nduca wrote: > Use the filename that the script provides. The automation harness (or the user) > can put in the filename themselves, rather than having Chrome prompt Makes it even easier. Will do.
I renamed the measurePaintTime method to just 'paint'.
http://codereview.chromium.org/10787025/diff/1/content/renderer/gpu/gpu_bench... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/1/content/renderer/gpu/gpu_bench... content/renderer/gpu/gpu_benchmarking_extension.cc:38: "chrome.gpuBenchmarking.measureRecordTime = function(dump) {" On 2012/07/16 22:38:09, Alok Priyadarshi wrote: > On 2012/07/16 22:28:40, nduca wrote: > > How about > > > > dumpToSkPicture = function(filename) > > > > I chose this name because it seemed more appropriate for a class named > Benchmarking. dump/printToSkPicture is OK too. There's a sister patch for all rending microbenchmarks, which includes measuring paint times to different canvases, so measuring the paint time here might be redundant.
Uploaded new patch. This will be ready to commit after WK88271 has landed and rolled. Another SKIA patch needs to be landed and rolled as well.
http://codereview.chromium.org/10787025/diff/7003/content/renderer/gpu/gpu_be... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/7003/content/renderer/gpu/gpu_be... content/renderer/gpu/gpu_benchmarking_extension.cc:174: return new GpuBenchmarkingWrapper(); does this get deleted somewhere?
http://codereview.chromium.org/10787025/diff/10001/content/renderer/gpu/gpu_b... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/10001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:36: } do we use the OVERRIDE convention yet in chromium? I think we might... http://codereview.chromium.org/10787025/diff/10001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:43: }; Is this the right way to handle layers? You're going to end up with one giant picture with TONS of layers in them... http://codereview.chromium.org/10787025/diff/10001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:143: SkFILEWStream file(*filename); We definitely want to file a bug with skia to allow serialization to memory instead of a file. We'll need this for your "phase 2" patch. http://codereview.chromium.org/10787025/diff/10001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:145: return v8::Number::New(record_time); Any reason we need to even return this variable? Seems we could just return undefined and that'd be ok.
http://codereview.chromium.org/10787025/diff/10001/content/renderer/gpu/gpu_b... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/10001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:36: } On 2012/07/18 06:17:47, nduca wrote: > do we use the OVERRIDE convention yet in chromium? I think we might... Will check. http://codereview.chromium.org/10787025/diff/10001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:43: }; On 2012/07/18 06:17:47, nduca wrote: > Is this the right way to handle layers? You're going to end up with one giant > picture with TONS of layers in them... We can do it in any number of ways: 1. multiple layers in one picture (current approach) 2. one picture per layer. all pictures saved into one composite picture. 3. one picture per layer. all pictures saved into separate files. We can start with this approach to be adjusted later as needed. http://codereview.chromium.org/10787025/diff/10001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:143: SkFILEWStream file(*filename); On 2012/07/18 06:17:47, nduca wrote: > We definitely want to file a bug with skia to allow serialization to memory > instead of a file. We'll need this for your "phase 2" patch. They already have this feature. We should sync up on "phase 2". http://codereview.chromium.org/10787025/diff/10001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:145: return v8::Number::New(record_time); On 2012/07/18 06:17:47, nduca wrote: > Any reason we need to even return this variable? Seems we could just return > undefined and that'd be ok. The same reason WebViewBenchmarkSupport::paint() returns paint time. It might be useful in certain situations. But we can also remove it for now and add it later if needed. Your call.
Thansks for the other clarifications. > The same reason WebViewBenchmarkSupport::paint() returns paint time. It might be > useful in certain situations. But we can also remove it for now and add it later > if needed. Your call. Lets stick to "exactly what we need and no more" for now. Seem okay?
Ima LGTM, though I think the underlying patch you're waiting on has changed a wee bit since you took it.
On 2012/07/20 06:43:18, nduca wrote: > Ima LGTM, though I think the underlying patch you're waiting on has changed a > wee bit since you took it. All DONE. Ready to check-in.
lgtm http://codereview.chromium.org/10787025/diff/15001/content/renderer/gpu/gpu_b... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/15001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:34: // WebViewBenchmarkSupport::PaintingController overrides. Nit: unnecessary + old comment
Uploaded a new patch to account for the new WebViewBenchmarkSupport::PaintClient API. The new patch also dumps multiple SkPictures - one picture per layer.
James/Antoine: Need owners approval.
http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:33: virtual WebCanvas* willPaint(const WebSize& size) OVERRIDE { If you are implementing something from the WebKit API do not use OVERRIDE, it makes doing rolls and API changes extremely difficult. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:36: virtual void didPaint(WebCanvas* canvas) OVERRIDE { newline before new function http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:36: virtual void didPaint(WebCanvas* canvas) OVERRIDE { No OVERRIDE http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:42: std::ostringstream stream; chromium style is to not use streams for things like this (this is inherited from the Google C++ style guide). there are many string building utilities in base/ that are probably useful for this sort of thing
http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:25: namespace { nit: blank line below http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:28: explicit SkPictureRecorder(const std::string& dirname) : nit: style - ':' goes on next line http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:35: } nit: blank line below http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:41: // --no-sandbox command-line flag. What could we do to remove that restriction? http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:43: stream << dirname_ << '/' << "layer_" << layer_id_++ << ".skp"; use FilePath instead of streams (prohibited) to construct the path. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:53: }; nit: blank line below
http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:25: namespace { On 2012/07/27 17:55:58, piman wrote: > nit: blank line below Done. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:28: explicit SkPictureRecorder(const std::string& dirname) : On 2012/07/27 17:55:58, piman wrote: > nit: style - ':' goes on next line Done. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:33: virtual WebCanvas* willPaint(const WebSize& size) OVERRIDE { On 2012/07/27 17:54:36, jamesr wrote: > If you are implementing something from the WebKit API do not use OVERRIDE, it > makes doing rolls and API changes extremely difficult. Done. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:35: } On 2012/07/27 17:55:58, piman wrote: > nit: blank line below Done. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:36: virtual void didPaint(WebCanvas* canvas) OVERRIDE { On 2012/07/27 17:54:36, jamesr wrote: > newline before new function Done. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:36: virtual void didPaint(WebCanvas* canvas) OVERRIDE { On 2012/07/27 17:54:36, jamesr wrote: > No OVERRIDE Done. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:41: // --no-sandbox command-line flag. On 2012/07/27 17:55:58, piman wrote: > What could we do to remove that restriction? The benchmarks could also be run on a picture serialized in memory. Serializing to file is mainly for the skia team to run their benchmarks. But addressing this limitation would be convenient. Please let me know if you have suggestions. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:42: std::ostringstream stream; On 2012/07/27 17:54:36, jamesr wrote: > chromium style is to not use streams for things like this (this is inherited > from the Google C++ style guide). there are many string building utilities in > base/ that are probably useful for this sort of thing Done. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:43: stream << dirname_ << '/' << "layer_" << layer_id_++ << ".skp"; On 2012/07/27 17:55:58, piman wrote: > use FilePath instead of streams (prohibited) to construct the path. FilePath may be an overkill for this use-case. I specifically do not want to deal with all the unicode-ascii conversions. I got rid of streams by using a string utility function in base. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:53: }; On 2012/07/27 17:55:58, piman wrote: > nit: blank line below Done.
http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:41: // --no-sandbox command-line flag. On 2012/07/27 18:32:44, Alok Priyadarshi wrote: > On 2012/07/27 17:55:58, piman wrote: > > What could we do to remove that restriction? > > The benchmarks could also be run on a picture serialized in memory. Serializing > to file is mainly for the skia team to run their benchmarks. > > But addressing this limitation would be convenient. Please let me know if you > have suggestions. For example, using the file picker. I won't block the CL on that, but please add a TODO and file a bug. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:43: stream << dirname_ << '/' << "layer_" << layer_id_++ << ".skp"; On 2012/07/27 18:32:44, Alok Priyadarshi wrote: > On 2012/07/27 17:55:58, piman wrote: > > use FilePath instead of streams (prohibited) to construct the path. > > FilePath may be an overkill for this use-case. I specifically do not want to > deal with all the unicode-ascii conversions. I got rid of streams by using a > string utility function in base. http://www.chromium.org/developers/coding-style explicitly calls for FilePath for all paths.
http://codereview.chromium.org/10787025/diff/31001/content/renderer/gpu/gpu_b... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/31001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:139: static v8::Handle<v8::Value> PrintToSkPicture(const v8::Arguments& args) { We should check if we have been launched with --no-sandbox and if not, then throw an Error back to v8.
http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:41: // --no-sandbox command-line flag. On 2012/07/27 20:59:31, piman wrote: > On 2012/07/27 18:32:44, Alok Priyadarshi wrote: > > On 2012/07/27 17:55:58, piman wrote: > > > What could we do to remove that restriction? > > > > The benchmarks could also be run on a picture serialized in memory. > Serializing > > to file is mainly for the skia team to run their benchmarks. > > > > But addressing this limitation would be convenient. Please let me know if you > > have suggestions. > > For example, using the file picker. > I won't block the CL on that, but please add a TODO and file a bug. May be I am missing something. The issue here is writing to a local file with sandbox enabled. How would the file picker get through the sandbox? Added TODO and crbug.com/139640. http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:43: stream << dirname_ << '/' << "layer_" << layer_id_++ << ".skp"; On 2012/07/27 20:59:31, piman wrote: > On 2012/07/27 18:32:44, Alok Priyadarshi wrote: > > On 2012/07/27 17:55:58, piman wrote: > > > use FilePath instead of streams (prohibited) to construct the path. > > > > FilePath may be an overkill for this use-case. I specifically do not want to > > deal with all the unicode-ascii conversions. I got rid of streams by using a > > string utility function in base. > > http://www.chromium.org/developers/coding-style explicitly calls for FilePath > for all paths. Done.
On 2012/07/30 18:05:12, Alok Priyadarshi wrote: > http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... > File content/renderer/gpu/gpu_benchmarking_extension.cc (right): > > http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... > content/renderer/gpu/gpu_benchmarking_extension.cc:41: // --no-sandbox > command-line flag. > On 2012/07/27 20:59:31, piman wrote: > > On 2012/07/27 18:32:44, Alok Priyadarshi wrote: > > > On 2012/07/27 17:55:58, piman wrote: > > > > What could we do to remove that restriction? > > > > > > The benchmarks could also be run on a picture serialized in memory. > > Serializing > > > to file is mainly for the skia team to run their benchmarks. > > > > > > But addressing this limitation would be convenient. Please let me know if > you > > > have suggestions. > > > > For example, using the file picker. > > I won't block the CL on that, but please add a TODO and file a bug. > > May be I am missing something. The issue here is writing to a local file with > sandbox enabled. How would the file picker get through the sandbox? > > Added TODO and crbug.com/139640. The file picker can whitelist the picked file(s) to a given renderer, and can provide a file descriptor you can write into from the renderer. > > http://codereview.chromium.org/10787025/diff/25001/content/renderer/gpu/gpu_b... > content/renderer/gpu/gpu_benchmarking_extension.cc:43: stream << dirname_ << '/' > << "layer_" << layer_id_++ << ".skp"; > On 2012/07/27 20:59:31, piman wrote: > > On 2012/07/27 18:32:44, Alok Priyadarshi wrote: > > > On 2012/07/27 17:55:58, piman wrote: > > > > use FilePath instead of streams (prohibited) to construct the path. > > > > > > FilePath may be an overkill for this use-case. I specifically do not want to > > > deal with all the unicode-ascii conversions. I got rid of streams by using a > > > string utility function in base. > > > > http://www.chromium.org/developers/coding-style explicitly calls for FilePath > > for all paths. > > Done.
LGTM after you address Nat's observation. Rather than testing for the presence of the --no-sandbox flag, maybe raise the exception if writing to the file failed?
WRT file picker discussion, the main use case of this is from an automation system. E.g. save a thousand websites to disk. So, the picker stuff may not be as useful in that context... I think the salient thing is what alok filed --- addressing the sandbox dependency.
Off to the trybots. http://codereview.chromium.org/10787025/diff/31001/content/renderer/gpu/gpu_b... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): http://codereview.chromium.org/10787025/diff/31001/content/renderer/gpu/gpu_b... content/renderer/gpu/gpu_benchmarking_extension.cc:139: static v8::Handle<v8::Value> PrintToSkPicture(const v8::Arguments& args) { DONE (in a slightly different way). I am throwing exception if the file-path is not writable.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alokp@chromium.org/10787025/32003
Change committed as 149287 |
