initial rendering microbenchmark
Created attachment 145662 [details] Patch
Comment on attachment 145662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145662&action=review Good first pass. Lets start gluing it all together. > Source/WebKit/chromium/public/RenderingBenchmarkResults.h:1 > +/* copy the style of other headers in that folder. > Source/WebKit/chromium/public/RenderingBenchmarkResults.h:10 > +#pragma once follow the style in the rest of the public/ folder > Source/WebKit/chromium/public/RenderingBenchmarkResults.h:12 > +class RenderingBenchmarkResults { We put "web" in the front of public wk apis. See other things in the folder, e.g. WebCompositorInputHandler.h > Source/WebKit/chromium/public/RenderingBenchmarkResults.h:15 > + virtual void addResult(const char *resultName, double result) = 0; A const char* for the unit is probably also a good thing. > Source/WebKit/chromium/public/WebView.h:486 > + // Rendering benchmarks ------------------------------------------------------ Lets put this entry point with the 'GPU acceleration support' group of methods, e.g. just above visibility after transferActiveWheelBlahblah Take a peek through this file and see if we typically pass by pointer. Pass-by-ref seems to be used only for pass-by-reference, though I might be missing something. > Source/WebKit/chromium/src/WebViewImpl.cpp:1614 > +void WebViewImpl::runRenderingBenchmarks(RenderingBenchmarkResults& results) Lets get this factored into a class like we described... RenderingBenchmark { const char* name() const = 0; void setUp(WebViewImpl* webViewImpl) {} void run(RenderingBenchmarkResults& results, WebViewImpl* webViewImpl) = 0; void tearDown(WebViewImpl* webViewImpl) { } } Then have another class called AllRenderingBenchmarks : RenderingBenchmark { AllRenderingBenchmarks() { push instances benchmark subclasses of interest into m_benchmarks; } run() { run each of the instances; } }; This runRenderingBenchamrks function should just construct a benchmark and run it. Ideally the test code will be in a bunch of classes in a cpp file --- the only thing exported by the benchmark.h would be the AllRenderingBenchmarks. > Source/WebKit/chromium/src/WebViewImpl.cpp:1623 > + this->paint(&canvas, screenRect); This works if you're in software mode. You need to handle accelerated mode as well. That is, you need to get the root graphics layer and iterate over each graphics layer painting it. > Source/WebKit/chromium/src/WebViewImpl.cpp:1625 > + results.addResult("paint to bitmap time", afterPaint - beforePaint); methinks the benchmark names should be camelcased words. E.g. paintTime > Source/WebKit/chromium/src/WebViewImpl.cpp:1632 > + results.addResult("paint to null canvas time", afterPaint - beforePaint); paintTime_NullCanvas
Created attachment 146072 [details] Patch
This patch separates the benchmarks into the AllRenderingBenchmarks.cpp file. A sister chromium patch will be uploaded this afternoon with the hookup from the benchmarking extension. Next, I'll be throwing an interface in between the WebViewImpl and the benchmarks, and expose the m_rootGraphicsLayer in that interface so we can simulate the accelerated graphics.
chromium sister patch: https://chromiumcodereview.appspot.com/10537036/
Created attachment 147618 [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.
FYI, we don't use std::auto_ptr in WebKit (or chromium) code.
For reviewers who are not Nat: Sorry, this patch wasn't meant to be reviewed by people yet, I specified '--no-review' when I uploaded but maybe it didn't take? This patch includes the 'controller' interface for the benchmarks, and has benchmarking for the accelerated mode. Things I was unsure of: * I have auto_ptr's in here for exception-safe memory management. I tried using the RefPtr, but that required deriving from a base, which was infeasible for WebCanvas (SkCanvas) and SkDevice. * I use 0,0,width,height to draw the entire layer, and I'm not sure if there's an offset I should be using. Shouldn't matter after next iteration. Still To-Do: * Find the correct clip rectangles for each layer instead of drawing them entirely. * Perhaps move everything into 'content', because we already have the 'controller' abstraction that we can leave in WebKit
We don't use C++ exceptions either, so don't worry about exception safety :)
Ok cool, I'll remove those then
Comment on attachment 147618 [details] Patch Hey Daniel - can you take a crack at making the controller a public interface on webkit, and moving the actual AllBenchmarks to chrome's content/renderer folder, perthaps alongside teh benchmarking extension? I think Darin was onto something here. :) Based on some of the discussion you heard in today's meeting, I think it will be useful to expose basic primitives like "paint the page and use this functor to create canvas objects." That would allow us to use the benchmark controller to do both microbenchmarking and this record thing.
Created attachment 148609 [details] Patch
Comment on attachment 148609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148609&action=review > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:41 > +// Interface between the rendering benchmarks and the WebViewImpl. and the WebView. > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:44 > + enum PaintMode { check webkit style guide on enums. barring anything there, grep for enums. I think we use different style. > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:48 > + class WebCanvasFactory { I think you can call this CanvasFactory since its inner. > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:54 > + WebRenderingBenchmarkController() { } Why have a public constructor? Seems like you dont ened a constructor at all, or if you do, it should be protected. > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:58 > + virtual double timePaint(WebCanvasFactory*, const PaintMode) = 0; Lets come up with a better name. measurePaintTIme Does this need layout called beforehand or does it do it automatically? You need much better docs explaining what this does. Also, I think you can pass a reference to the factory rather than a pointer. That clarifies ownership. > Source/WebKit/chromium/public/WebView.h:459 > + virtual WebRenderingBenchmarkController* createRenderingBenchmarkController() Caller assumes ownership? Why? I think WebViewImpl should ownptr<renderingBenchmarkConttroller> (lazily create it) and this can just be an accessor. > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:1 > +/* This should be called xxximpl. See for example webcompositorinputhandler and webcompositorinputhandlerimpl Eg WebRenderingBenchmarkControllerImpl > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:48 > +double WebViewImplRenderingBenchmarkController::timePaint(WebCanvasFactory* canvasFactory, const PaintMode paintMode) The public header should document the units returned. > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:50 > + m_webViewImpl->layout(); Why expose layout if you're automatically laying out? Dont expose it if its not serving a purpose. :) > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:53 > + WebCore::GraphicsLayer* layer = m_webViewImpl->rootGraphicsLayer(); this should probably be split out to a standalone function > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:61 > + layers.prepend(layer->children()); why are you prepending? you realize that this is a vector so each prepend is super inefficient? > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:67 > + case PAINT_VIEWPORT: // TODO(dmurph): implement this You should assert not reached in this case. > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:89 > + WebSize size = m_webViewImpl->size(); this could be another fucntion > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:110 > + WebCanvas* canvas = canvasFactory->createWebCanvas(canvasSize); You should have OwnPtr<WebCanvas> canvas = adoptPtr(canvasFactory->create(...)); that way you never leak > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:111 > + double beforeTime = currentTime(); you should be using monotonicallyIncreasingTime not currentTime.
Comment on attachment 148609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148609&action=review >> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:44 >> + enum PaintMode { > > check webkit style guide on enums. barring anything there, grep for enums. I think we use different style. http://trac.webkit.org/wiki/ChromiumWebKitAPI has a section on enums
(In reply to comment #14) > > Source/WebKit/chromium/public/WebView.h:459 > > + virtual WebRenderingBenchmarkController* createRenderingBenchmarkController() > > Caller assumes ownership? Why? > > I think WebViewImpl should ownptr<renderingBenchmarkConttroller> (lazily create it) and this can just be an accessor. That's fine as long as the controller is stateless. Otherwise, it needs to be a factory. Do you want to impose the stateless constraint on the controller, or the ownership constraint on the WebView? > > > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:50 > > + m_webViewImpl->layout(); > > Why expose layout if you're automatically laying out? Dont expose it if its not serving a purpose. :) I'm using it, I have a benchmark that just calls layout. I can remove that. > > > Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:61 > > + layers.prepend(layer->children()); > > why are you prepending? you realize that this is a vector so each prepend is super inefficient? Obviously, yes, I just couldn't find a doubly-linked list in WTF and I need to prepend for breadth-first-search (which makes applying layer transformations easier). I just talked to Julian and he pointed me towards Deque, which looks like it doesn't copy on prepend
I think you're overthinking the stateless thing. I think that the functions should do what they say they're going to do regardless of what was called on them in the past. So, for example, it is good that layout NOT be on the interface and that it internally do layout for you. Anyway, I'm not clear how you "impose a stateless constraint"? Wrt deque, you could also write a recursive function. :)
(In reply to comment #17) > I think you're overthinking the stateless thing. I think that the functions should do what they say they're going to do regardless of what was called on them in the past. So, for example, it is good that layout NOT be on the interface and that it internally do layout for you. Anyway, I'm not clear how you "impose a stateless constraint"? > > Wrt deque, you could also write a recursive function. :) By stateless I meant there wouldn't be conflicts if two people were using the same controller. I think it's better overall that the controller be stateless. So, WebView owning it is cool. I might use recursion if it makes the code simpler
Created attachment 150035 [details] Patch
Fixes for commented issues
This LGTM mod time->measure. JamesR for review.
Created attachment 150496 [details] Patch
Fixed time->measure, cleanup up accelerated check in the benchmark impl.
Attachment 150496 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/ChangeLog:4: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 150497 [details] Patch
Fixed tab character in ChangeLog file
Comment on attachment 150496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150496&action=review I've left some feedback that is hopefully helpful. Unfortunately I'm heading out now and will be gone for a few weeks, so I suggest you find another public API reviewer to work with in order to get this in landable shape. > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:36 > +#include "platform/WebSize.h" If the only WebSize use is a const reference then you could just use a forward declaration. Alternately, since WebSize is two ints, passing by value is probably fine. > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:41 > +class WebRenderingBenchmarkController { I don't see what about this class is a 'controller'. When seeing a WebRenderingBenchmarkController, I expect to see a WebRenderingBenchmark type somewhere that this controls. Perhaps this could have a shorter, more direct name? > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:43 > + enum PaintMode { This enum is mis-named, see the section on enums in http://trac.webkit.org/wiki/ChromiumWebKitAPI > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:49 > + class CanvasFactory { this needs some explanation of what it's doing and what the expectations are of the implementation. a factory pattern is fairly unusual in the WebKit public API > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:51 > + virtual ~CanvasFactory() { } should this be public? i.e. does the WebRenderingBenchmarkController take ownership of the factory, or should that be hidden from the controller? (I suspect the answer is the latter, which means this should probably be protected). > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:56 > + virtual ~WebRenderingBenchmarkController() { } Should this be public (i.e. should someone be able to delete a WebRenderingBenchmarkController via a WebRenderingBenchmarkController* if they have no knowledge of the underlying type)? the answer is usually no > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:58 > + // Returns the time in seconds to paint on given canvas/es. The comment is a bit misleading since this function isn't actually given a canvas. > Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:59 > + virtual double measurePaintTime(const CanvasFactory&, const PaintMode) const = 0; No point in having 'const' on by-value parameters like an enum, it's copied anyway. > Source/WebKit/chromium/public/WebView.h:466 > + // Rending Benchmark support -------------------------------------------- typo - rendering ? > Source/WebKit/chromium/public/WebView.h:469 > + return 0; 4-space indentation, or just put this on one line > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:8 > + * * Redistributions of source code must retain the above copyright we prefer 2-clause license for new files > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:38 > +#include "public/WebCanvas.h" <public/WebCanvas.h>, please > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:48 > +using WTF::OwnPtr; > +using WTF::Vector; You don't need this, OwnPtr.h / Vector.h have using declarations that put OwnPtr/Vector in the global namespace > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:52 > +#if USE(ACCELERATED_COMPOSITING) Chromium always sets USE(ACCELERATED_COMPOSITING), there's no point in guarding stuff in /chromium/ paths with this guard > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:56 > + const OwnPtr<WebCanvas> canvas = WTF::adoptPtr(canvasFactory.createWebCanvas(canvasSize)); no WTF:: > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:61 > + return monotonicallyIncreasingTime() - beforeTime; are you trying to isolate the time spent painting from the time spent creating/destroying the canvases? This might accumulate a lot of error if you have many small layers with cheap paint times > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:66 > + // TODO(dmurph): go through all the layers, using the internal WebLayer We don't use TODO(name) in WebKit, just FIXME - no name. Or even better, file a bug and link to it. Or even better better, don't land this code until it works. Code in the tree is code everybody working on the project has to worry about, so there's not much use in landing code that doesn't work yet. > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:75 > + const FloatSize layerSize = layer.size(); we don't typically use const for locals like this. If the function is big and long and complicated having const on locals can help you figure out invariants, but a better solution to that is often to just make the function less big + long + complicated. I'd just leave the const out here. > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:104 > + default: better to leave out the default: completely and have the compiler check that you have a case: for every possible value enum value > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:110 > + const OwnPtr<WebCanvas> canvas = WTF::adoptPtr(canvasFactory.createWebCanvas(canvasSize)); no WTF:: > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:131 > + default: same here re: default > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:35 > +#include "GraphicsLayer.h" > +#include "IntRect.h" Forward declarations of these types would suffice > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:42 > + WebRenderingBenchmarkControllerImpl(WebViewImpl* webViewImpl) : m_webViewImpl(webViewImpl) { } explicit on one-arg constructors > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:53 > + // returns the time in seconds please start your comments with an uppercase letter and end them with a period. for bonus points, have comments be completely sentences. > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:54 > + double measureLayerPaint(const CanvasFactory&, WebCore::GraphicsLayer&, const WebCore::IntRect&) const; why is GraphicsLayer being passed by non-const reference? is it an out parameter? > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:55 > + // returns the time in seconds. Currently unimplemented. why land it if it's unimplemented? probably better to just not land it until it works > Source/WebKit/chromium/src/WebViewImpl.cpp:749 > + if (!m_renderingBenchmarkController.get()) all WTF smart pointers provide operator bool() overrides that do the right thing, so this check should just be written: if (!m_renderingBenchmarkController) > Source/WebKit/chromium/src/WebViewImpl.h:300 > + virtual const WebRenderingBenchmarkController* renderingBenchmarkController(); we don't typically use const for this
> > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:55 > > + // returns the time in seconds. Currently unimplemented. > > why land it if it's unimplemented? probably better to just not land it until it works Because we need the infrastructure for things before we need the viewport version. I think this is acceptable.
Created attachment 151547 [details] Patch
Comment on attachment 150496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150496&action=review >> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:41 >> +class WebRenderingBenchmarkController { > > I don't see what about this class is a 'controller'. When seeing a WebRenderingBenchmarkController, I expect to see a WebRenderingBenchmark type somewhere that this controls. Perhaps this could have a shorter, more direct name? Renamed to WebViewBenchmarkSupport >> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:43 >> + enum PaintMode { > > This enum is mis-named, see the section on enums in http://trac.webkit.org/wiki/ChromiumWebKitAPI I think you might not be seeing the most recent patch? This follows the guide. >> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:49 >> + class CanvasFactory { > > this needs some explanation of what it's doing and what the expectations are of the implementation. a factory pattern is fairly unusual in the WebKit public API Done. >> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:51 >> + virtual ~CanvasFactory() { } > > should this be public? i.e. does the WebRenderingBenchmarkController take ownership of the factory, or should that be hidden from the controller? (I suspect the answer is the latter, which means this should probably be protected). I haven't seen that pattern before, pretty cool. Done. >> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:56 >> + virtual ~WebRenderingBenchmarkController() { } > > Should this be public (i.e. should someone be able to delete a WebRenderingBenchmarkController via a WebRenderingBenchmarkController* if they have no knowledge of the underlying type)? the answer is usually no Done. >> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:58 >> + // Returns the time in seconds to paint on given canvas/es. > > The comment is a bit misleading since this function isn't actually given a canvas. Done. >> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:59 >> + virtual double measurePaintTime(const CanvasFactory&, const PaintMode) const = 0; > > No point in having 'const' on by-value parameters like an enum, it's copied anyway. Done. >> Source/WebKit/chromium/public/WebView.h:466 >> + // Rending Benchmark support -------------------------------------------- > > typo - rendering ? Done. >> Source/WebKit/chromium/public/WebView.h:469 >> + return 0; > > 4-space indentation, or just put this on one line Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:8 >> + * * Redistributions of source code must retain the above copyright > > we prefer 2-clause license for new files Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:38 >> +#include "public/WebCanvas.h" > > <public/WebCanvas.h>, please Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:48 >> +using WTF::Vector; > > You don't need this, OwnPtr.h / Vector.h have using declarations that put OwnPtr/Vector in the global namespace Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:52 >> +#if USE(ACCELERATED_COMPOSITING) > > Chromium always sets USE(ACCELERATED_COMPOSITING), there's no point in guarding stuff in /chromium/ paths with this guard Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:56 >> + const OwnPtr<WebCanvas> canvas = WTF::adoptPtr(canvasFactory.createWebCanvas(canvasSize)); > > no WTF:: Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:61 >> + return monotonicallyIncreasingTime() - beforeTime; > > are you trying to isolate the time spent painting from the time spent creating/destroying the canvases? This might accumulate a lot of error if you have many small layers with cheap paint times This is intentional. Perhaps in the future we'll measure the creation as well, but for now, it's just the paint. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:66 >> + // TODO(dmurph): go through all the layers, using the internal WebLayer > > We don't use TODO(name) in WebKit, just FIXME - no name. Or even better, file a bug and link to it. Or even better better, don't land this code until it works. Code in the tree is code everybody working on the project has to worry about, so there's not much use in landing code that doesn't work yet. I agree, I think I'll remove the enumeration value then (and these methods), but keep the switch statement architecture for the next patch >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:75 >> + const FloatSize layerSize = layer.size(); > > we don't typically use const for locals like this. If the function is big and long and complicated having const on locals can help you figure out invariants, but a better solution to that is often to just make the function less big + long + complicated. I'd just leave the const out here. Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:104 >> + default: > > better to leave out the default: completely and have the compiler check that you have a case: for every possible value enum value Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:110 >> + const OwnPtr<WebCanvas> canvas = WTF::adoptPtr(canvasFactory.createWebCanvas(canvasSize)); > > no WTF:: Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:131 >> + default: > > same here re: default Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:35 >> +#include "IntRect.h" > > Forward declarations of these types would suffice Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:42 >> + WebRenderingBenchmarkControllerImpl(WebViewImpl* webViewImpl) : m_webViewImpl(webViewImpl) { } > > explicit on one-arg constructors Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:53 >> + // returns the time in seconds > > please start your comments with an uppercase letter and end them with a period. for bonus points, have comments be completely sentences. Done. >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:54 >> + double measureLayerPaint(const CanvasFactory&, WebCore::GraphicsLayer&, const WebCore::IntRect&) const; > > why is GraphicsLayer being passed by non-const reference? is it an out parameter? No, but the paint call is non-const >> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:55 >> + // returns the time in seconds. Currently unimplemented. > > why land it if it's unimplemented? probably better to just not land it until it works I agree, removed. >> Source/WebKit/chromium/src/WebViewImpl.cpp:749 >> + if (!m_renderingBenchmarkController.get()) > > all WTF smart pointers provide operator bool() overrides that do the right thing, so this check should just be written: > > if (!m_renderingBenchmarkController) Done. >> Source/WebKit/chromium/src/WebViewImpl.h:300 >> + virtual const WebRenderingBenchmarkController* renderingBenchmarkController(); > > we don't typically use const for this const here enforces that WebRenderingBenchmarkController can be safely accessed by multiple people. The controller should not be able to be changed by the caller.
Comment on attachment 151547 [details] Patch Attachment 151547 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13203131
Created attachment 151556 [details] Patch
Comment on attachment 151556 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151556&action=review Lets see what @fishd thinks. > Source/WebKit/chromium/ChangeLog:3 > + initial microbenchmark web view controller Lets update this comment to be a bit more clear. E.g. "Introduce WebViewBenchmarkSupport as a way to do performance experiments on a real WebView" > Source/WebKit/chromium/public/WebView.h:466 > + // Benchmark support -------------------------------------------- // Benchmarking ---- ^^ instead of benchmark support? > Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:52 > + virtual WebCanvas* createWebCanvas(const WebSize&) const = 0; I dont think this needs to be a const method. What if I wanted to count the number of canvases created? Or track the area of paints? That requires that the factory is non-const. > Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:59 > + virtual double measurePaintTime(const CanvasFactory&, PaintMode) const = 0; Does this need to be passed by const? I think thats unnecessarily restrictive. > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:101 > +#if USE(ACCELERATED_COMPOSITING) Did we decide to nix this #if?
Comment on attachment 151556 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151556&action=review > Source/WebKit/chromium/public/WebView.h:467 > + virtual const WebViewBenchmarkSupport* benchmarkSupport() { return 0; } This shouldn't return a const WVBS*. It should be straight WebViewBenchmarkSupport. Sorry, I dont buy your previous comment about enforcing anything.
Comment on attachment 151556 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151556&action=review > Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:43 > + enum PaintMode { do you need this enum? do you plan on adding more values here in the future? > Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:44 > + // Paint the entire page. nit: indentation > Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:47 > + // Factory for creating canvases for paint calls, where multiple canvases you might also mention that the factory makes it convenient for the caller to not have to know/compute the size of the canvas up front. > Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:51 > + // Creates a web canvas, caller assumes ownership. nit: this comment is self-evident. you can leave it out. the "create" prefix implies the ownership transfer, and this function's name makes it clear that it is creating a canvas. >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:52 >> + virtual WebCanvas* createWebCanvas(const WebSize&) const = 0; > > I dont think this needs to be a const method. What if I wanted to count the number of canvases created? Or track the area of paints? That requires that the factory is non-const. nit: we usually leave off the "Web" in method names that refer to types. createCanvas, returning WebCanvas*, would be a fine name for this method that is consistent with existing code. > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:65 > + WebViewImpl* m_webViewImpl; notice how WebViewBenchmarkSupportImpl has no real member variables other than the backpointer to the WebViewImpl? perhaps allocating it lazily does not really offer much benefit? i suppose it trims the size of WebViewImpl by a DWORD doing it this way. however, if you wanted to just have WebViewImpl have a non-pointer member variable of type WebViewBenchmarkSupportImpl that would be OK too. > Source/WebKit/chromium/src/WebViewImpl.h:301 > + virtual const WebViewBenchmarkSupport* benchmarkSupport(); nit: no need for the new line between this method declaration and renderingStats. the idea is that methods should be listed in the same order as they are declared in the interface being implemented. the .cpp file should similarly list the methods in order. (WebViewImpl seems to have some mistakes in this regard.)
Created attachment 152123 [details] Patch
Comment on attachment 151556 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151556&action=review >> Source/WebKit/chromium/public/WebView.h:466 >> + // Benchmark support -------------------------------------------- > > // Benchmarking ---- > ^^ instead of benchmark support? Done. >> Source/WebKit/chromium/public/WebView.h:467 >> + virtual const WebViewBenchmarkSupport* benchmarkSupport() { return 0; } > > This shouldn't return a const WVBS*. It should be straight WebViewBenchmarkSupport. Sorry, I dont buy your previous comment about enforcing anything. Done. >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:43 >> + enum PaintMode { > > do you need this enum? do you plan on adding more values here in the future? We're adding another paint mode in the next cl >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:44 >> + // Paint the entire page. > > nit: indentation Done. >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:47 >> + // Factory for creating canvases for paint calls, where multiple canvases > > you might also mention that the factory makes it convenient for the caller to not have > to know/compute the size of the canvas up front. Done. Also, renamed to CanvasController, as it will also have observer methods for when painting is about to occur and when painting is complete for a web canvas. >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:51 >> + // Creates a web canvas, caller assumes ownership. > > nit: this comment is self-evident. you can leave it out. the "create" prefix implies the ownership transfer, and this function's name makes it clear that it is creating a canvas. Ok >>> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:52 >>> + virtual WebCanvas* createWebCanvas(const WebSize&) const = 0; >> >> I dont think this needs to be a const method. What if I wanted to count the number of canvases created? Or track the area of paints? That requires that the factory is non-const. > > nit: we usually leave off the "Web" in method names that refer to types. createCanvas, returning WebCanvas*, would be a fine name for this method that is consistent with existing code. Done, Done. >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:59 >> + virtual double measurePaintTime(const CanvasFactory&, PaintMode) const = 0; > > Does this need to be passed by const? I think thats unnecessarily restrictive. Done. >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:101 >> +#if USE(ACCELERATED_COMPOSITING) > > Did we decide to nix this #if? Whoops, missed that one. fixed. >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:65 >> + WebViewImpl* m_webViewImpl; > > notice how WebViewBenchmarkSupportImpl has no real member variables other than > the backpointer to the WebViewImpl? perhaps allocating it lazily does not really > offer much benefit? i suppose it trims the size of WebViewImpl by a DWORD doing > it this way. however, if you wanted to just have WebViewImpl have a non-pointer > member variable of type WebViewBenchmarkSupportImpl that would be OK too. Good idea, done. >> Source/WebKit/chromium/src/WebViewImpl.h:301 >> + virtual const WebViewBenchmarkSupport* benchmarkSupport(); > > nit: no need for the new line between this method declaration and renderingStats. > > the idea is that methods should be listed in the same order as they are declared > in the interface being implemented. the .cpp file should similarly list the methods > in order. (WebViewImpl seems to have some mistakes in this regard.) Done.
So I forgot to call the painting report methods, I'll upload another patch tomorrow morning with those four lines added in WebViewBenchmarkSupportImpl, my bad.
Created attachment 152278 [details] Patch
Comment on attachment 152278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152278&action=review > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:77 > +double WebViewBenchmarkSupportImpl::measureSoftwarePaintTime(PaintingController& paintingController, const PaintMode paintMode) no need for "const" PaintMode. > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:103 > +double WebViewBenchmarkSupportImpl::measurePaintTime(PaintingController& paintingController, const PaintMode paintMode) same here
Comment on attachment 152278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152278&action=review >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:77 >> +double WebViewBenchmarkSupportImpl::measureSoftwarePaintTime(PaintingController& paintingController, const PaintMode paintMode) > > no need for "const" PaintMode. Done. >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:103 >> +double WebViewBenchmarkSupportImpl::measurePaintTime(PaintingController& paintingController, const PaintMode paintMode) > > same here Done.
Created attachment 152563 [details] Patch
Created attachment 152645 [details] Patch
Comment on attachment 152645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152645&action=review > Source/WebKit/chromium/public/WebView.h:454 > + // Benchmarking support -------------------------------------------- nit: add a new line below the section comment to be consistent with the rest of this file. > Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:45 > + PaintModeEverything it is hard to review the name of this enum without considering what the other enum values will be. PaintModeEverything reads a bit awkwardly. another option besides enums is to just have multiple entry points. for example, paint could be named paintEverything or paintAll. > Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:52 > + class PaintingController { This seems like more of a Client interface than a Controller interface. Since the method this goes with is named "paint", I'd probably call this interface PaintClient. > Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:56 > + // Called by the WebViewBenchmakrSupport when painting is about to typo: WebViewBenchmakrSupport I recommend using the more conventional names for these functions: willPaint and didPaint We use these prefixes to denote before and after events. > Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:68 > + virtual double paint(PaintingController&, PaintMode) = 0; nit: it is more conventional to pass callback interfaces by pointer. virtual double paint(PaintClient*, PaintMode) = 0; > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:51 > + const WebSize canvasSize = WebSize(clip.width(), clip.height()); nit: same comment about gratuitous consts and WebSize construction > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:66 > + const IntRect clip = WebCore::IntRect(0, 0, layerSize.width(), layerSize.height()); nit: should be "IntRect clip(...)" nit: isn't there a potential lossy conversion here from float to int? Or, can we assume that layer.size() always returns integer values? > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:92 > + const WebSize canvasSize = WebSize(paintSize.width, paintSize.height); nit: no need for 'const' on these local variables. it just adds noise. nit: instead of "WebSize foo = WebSize(...)," just do "WebSize foo(...)" > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:95 > + double beforeTime = monotonicallyIncreasingTime(); it occurs to me that the timing code could be implemented on the client side. in willPaint record the start time. in didPaint record the stop time. this means less code in webkit! > Source/WebKit/chromium/src/WebViewImpl.h:791 > + // Lazily created controller for microbenchmarks nit: this comment is no longer correct
Comment on attachment 152645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152645&action=review >> Source/WebKit/chromium/public/WebView.h:454 >> + // Benchmarking support -------------------------------------------- > > nit: add a new line below the section comment to be consistent with the rest > of this file. Done. >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:45 >> + PaintModeEverything > > it is hard to review the name of this enum without considering what the other > enum values will be. PaintModeEverything reads a bit awkwardly. another > option besides enums is to just have multiple entry points. for example, > paint could be named paintEverything or paintAll. The next enum will be PaintModeViewport. I'd rather use enums, and they make it cleaner on the implementation side as well. >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:52 >> + class PaintingController { > > This seems like more of a Client interface than a Controller interface. > Since the method this goes with is named "paint", I'd probably call this > interface PaintClient. Done. >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:56 >> + // Called by the WebViewBenchmakrSupport when painting is about to > > typo: WebViewBenchmakrSupport > > I recommend using the more conventional names for these functions: > willPaint and didPaint > > We use these prefixes to denote before and after events. Done. >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:68 >> + virtual double paint(PaintingController&, PaintMode) = 0; > > nit: it is more conventional to pass callback interfaces by pointer. > > virtual double paint(PaintClient*, PaintMode) = 0; Done. >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:51 >> + const WebSize canvasSize = WebSize(clip.width(), clip.height()); > > nit: same comment about gratuitous consts and WebSize construction Done. >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:66 >> + const IntRect clip = WebCore::IntRect(0, 0, layerSize.width(), layerSize.height()); > > nit: should be "IntRect clip(...)" > > nit: isn't there a potential lossy conversion here from float to int? > Or, can we assume that layer.size() always returns integer values? Done, I think we can assume layer.size() will always return integer values, I don't think there's another choice here either, we can only paint IntRects >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:92 >> + const WebSize canvasSize = WebSize(paintSize.width, paintSize.height); > > nit: no need for 'const' on these local variables. it just adds noise. > > nit: instead of "WebSize foo = WebSize(...)," just do "WebSize foo(...)" Done. >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:95 >> + double beforeTime = monotonicallyIncreasingTime(); > > it occurs to me that the timing code could be implemented on the client > side. in willPaint record the start time. in didPaint record the stop > time. this means less code in webkit! True, but we might want to add different time measurment modes in the future, James mentioned we might want to include the canvas creation at some point. Nat, would we ever want that? Or can we move the timing out of WebKit. >> Source/WebKit/chromium/src/WebViewImpl.h:791 >> + // Lazily created controller for microbenchmarks > > nit: this comment is no longer correct Done.
(In reply to comment #45) > >> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:45 > >> + PaintModeEverything > > > > it is hard to review the name of this enum without considering what the other > > enum values will be. PaintModeEverything reads a bit awkwardly. another > > option besides enums is to just have multiple entry points. for example, > > paint could be named paintEverything or paintAll. > > The next enum will be PaintModeViewport. I'd rather use enums, and they make it cleaner on the implementation side as well. This has come up a few times in the reivew. We had the enum fully filled out before but then james asked us to remove it because we weren't using it. Changing function arguments in the webkit api is very time consuming, so this was the compromise I was trying to have us make.
Created attachment 152900 [details] Patch
Removed timings
Comment on attachment 152900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152900&action=review > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:43 > +using WebCore::FloatSize; nit: I recommend a 'using namespace WebCore' here. > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:63 > + IntRect clip = WebCore::IntRect(0, 0, layerSize.width(), layerSize.height()); nit: IntRect clip(...) nit: no need for WebCore:: in front of IntRect due to the 'using WebCore::IntRect' statement. > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:57 > + void paintLayer(PaintClient*, WebCore::GraphicsLayer&, const WebCore::IntRect&); nit: insert new line between function and the next comment block. I think it would be helpful to name the last parameter "clip". then you could probably delete the comment for this function as it would be fairly redundant with the function signature. > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:61 > + // Paints the WebViewImpl using software rendering, with the given paint mode nit: this comment is fairly redundant with the method signature.
Created attachment 153054 [details] Patch
Comment on attachment 152900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152900&action=review >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:43 >> +using WebCore::FloatSize; > > nit: I recommend a 'using namespace WebCore' here. Done. >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:63 >> + IntRect clip = WebCore::IntRect(0, 0, layerSize.width(), layerSize.height()); > > nit: IntRect clip(...) > > nit: no need for WebCore:: in front of IntRect due to the 'using WebCore::IntRect' statement. Done. >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:57 >> + void paintLayer(PaintClient*, WebCore::GraphicsLayer&, const WebCore::IntRect&); > > nit: insert new line between function and the next comment block. > > I think it would be helpful to name the last parameter "clip". then you could > probably delete the comment for this function as it would be fairly redundant > with the function signature. Done. >> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:61 >> + // Paints the WebViewImpl using software rendering, with the given paint mode > > nit: this comment is fairly redundant with the method signature. Done.
Comment on attachment 153054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153054&action=review > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:76 > + case PaintModeEverything: { nit: no {}'s for this case. you aren't declaring a variable. > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:101 > + acceleratedPaintUnclipped(paintClient, *layer); nit: add a "break;" at the end of this case, so you don't have to remember to add one later when you add the other cases.
Created attachment 153561 [details] Patch
Created attachment 153570 [details] Patch for commit
Comment on attachment 153570 [details] Patch for commit Clearing flags on attachment: 153570 Committed r123254: <http://trac.webkit.org/changeset/123254>
All reviewed patches have been landed. Closing bug.
Comment on attachment 153570 [details] Patch for commit View in context: https://bugs.webkit.org/attachment.cgi?id=153570&action=review > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:50 > + OwnPtr<WebCanvas> canvas = adoptPtr(paintClient->createCanvas(canvasSize)); This will not work with a recording canvas. It cannot be destroyed here. PaintClient should be responsible for destroying the canvas. It is also better design to co-locate create and destroy. I will upload another patch.
Has this been tested on acclerated-composited content? I am hitting this assert with poster-circle example: http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayerBacking.cpp#L1217 poster-circle example: http://www.webkit.org/blog-files/3d-transforms/poster-circle.html
Interesting... I've been running with the compositor always on, I didn't test with that specific page though. This looks like it will require a bit of discussion with people working on the compositor
Tested, and I don't hit that assertion when I call paint for that page.
(In reply to comment #60) > Tested, and I don't hit that assertion when I call paint for that page. Hmm. Does not seem like the assert would depend on the type of canvas. Anyways I will ask you to test my patch once it is landed.
(In reply to comment #57) > (From update of attachment 153570 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153570&action=review > > > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:50 > > + OwnPtr<WebCanvas> canvas = adoptPtr(paintClient->createCanvas(canvasSize)); > > This will not work with a recording canvas. It cannot be destroyed here. PaintClient should be responsible for destroying the canvas. It is also better design to co-locate create and destroy. I will upload another patch. I think you should be precise here... it will work for one use case. Its just that it wont work if you had one-layer-per-canvas canvas to the file that you wanted to start with. If you write the picture after every didPaint, then you'd be fine, correct?
(In reply to comment #62) > (In reply to comment #57) > > (From update of attachment 153570 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=153570&action=review > > > > > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:50 > > > + OwnPtr<WebCanvas> canvas = adoptPtr(paintClient->createCanvas(canvasSize)); > > > > This will not work with a recording canvas. It cannot be destroyed here. PaintClient should be responsible for destroying the canvas. It is also better design to co-locate create and destroy. I will upload another patch. > > I think you should be precise here... it will work for one use case. Its just that it wont work if you had one-layer-per-canvas canvas to the file that you wanted to start with. > > If you write the picture after every didPaint, then you'd be fine, correct? No. It is not the issue of writing after every paint. Just ownership. The canvas returned by SkPicture is owned by SkPicture, and cannot be destroyed by anybody else. I have filed https://bugs.webkit.org/show_bug.cgi?id=92008. Will upload a patch as soon as prepare-changelogs cooperates.
Ah, that clarifies things. Thanks!