Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2141793002:
Improving canvas 2D performance by switching graphics rendering pipeline. (Closed)
|
Created:
4 years, 5 months ago by sebastienlc Modified:
4 years, 5 months ago CC:
achuith+watch_chromium.org, ajuma+watch-canvas_chromium.org, ajuma+watch_chromium.org, asvitkine+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, creis+watch_chromium.org, danakj+watch_chromium.org, darin-cc_chromium.org, davemoore+watch_chromium.org, dglazkov+blink, dshwang, drott+blinkwatch_chromium.org, krit, dzhioev+watch_chromium.org, f(malita), haraken, jam, jbroman, kinuko+watch, nasko+codewatch_chromium.org, oshima+watch_chromium.org, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImproving canvas 2D performance by switching graphics rendering pipeline.
1. Functionality to switch from the accelerated (GPU) graphics pipeline
to the recording graphics pipeline.
2. Simple heuristic to determine when the pipeline switch would increase
performance based on how the canvas was used previously.
3. Automatically switch pipeline to increase performance based on the
heuristic if the enable-canvas-2d-dynamic-pipeline-mode-switching flag is enabled.
BUG=606687, 606686, 606685
TBR=grt@chromium.org
Committed: https://crrev.com/98739b054a5b28cc607d688b2e9e582456bbb20e
Cr-Commit-Position: refs/heads/master@{#406585}
Patch Set 1 #
Total comments: 28
Patch Set 2 : Addressing comments #
Total comments: 16
Patch Set 3 : Addressing comments #Patch Set 4 : Style and comment typo #Patch Set 5 : Added flag to histograms.xml enum LoginCustomFlags. #
Total comments: 4
Patch Set 6 : Use canvas2dFixedRenderingModeEnabled flag to disable feature in some tests and fix async issue. #
Total comments: 2
Patch Set 7 : Changed comments #Patch Set 8 : Minor change #Messages
Total messages: 65 (41 generated)
Description was changed from ========== 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the feature is enabled by the enable-canvas-2d-dynamic-pipeline-mode flag. BUG= ========== to ========== 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the feature is enabled by the enable-canvas-2d-dynamic-pipeline-mode flag. BUG= ==========
sebastienlc@google.com changed reviewers: + junov@chromium.org
sebastienlc@google.com changed reviewers: + senorblanco@chromium.org
Description was changed from ========== 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the feature is enabled by the enable-canvas-2d-dynamic-pipeline-mode flag. BUG= ========== to ========== Improving canvas 2d performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the feature is enabled by the enable-canvas-2d-dynamic-pipeline-mode flag. BUG= ==========
Description was changed from ========== Improving canvas 2d performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the feature is enabled by the enable-canvas-2d-dynamic-pipeline-mode flag. BUG= ========== to ========== Improving canvas 2d performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the feature is enabled by the enable-canvas-2d-dynamic-pipeline-mode flag. BUG= ==========
Description was changed from ========== Improving canvas 2d performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the feature is enabled by the enable-canvas-2d-dynamic-pipeline-mode flag. BUG= ========== to ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the feature is enabled by the enable-canvas-2d-dynamic-pipeline-mode flag. BUG= ==========
Description was changed from ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the feature is enabled by the enable-canvas-2d-dynamic-pipeline-mode flag. BUG= ========== to ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode flag is enabled. BUG= ==========
Description was changed from ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline in the 2D canvas. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode flag is enabled. BUG= ========== to ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode flag is enabled. BUG= ==========
Looks great overall https://codereview.chromium.org/2141793002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2141793002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5441: + <message name="IDS_FLAGS_ENABLE_2D_CANVAS_DYNAMIC_PIPELINE_MODE_NAME" desc="Enable canvas 2D dynamic pipeline switching mode."> name -> IDS_FLAGS_ENABLE_2D_CANVAS_DYNAMIC_RENDERING_MODE_NAME desc -> Enable 2D canvas dynamic rendering mode switching. https://codereview.chromium.org/2141793002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5444: + <message name="IDS_FLAGS_ENABLE_2D_CANVAS_DYNAMIC_PIPELINE_MODE_DESCRIPTION" desc="Description of 'Enable canvas 2D dynamic pipeline switching mode'"> Same here. https://codereview.chromium.org/2141793002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5445: + Enables dynamic graphics rendering pipeline switching in the 2D canvas to increase performance. (...) to optimize performance based on the types of draw operations being invoked. https://codereview.chromium.org/2141793002/diff/1/content/public/common/conte... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2141793002/diff/1/content/public/common/conte... content/public/common/content_switches.cc:367: "enable-canvas-2d-dynamic-pipeline-mode"; pipeline->rendering Pipeline is a loaded word that could easily be misinterpred in this case, let's call this a rendering mode. https://codereview.chromium.org/2141793002/diff/1/content/public/common/conte... File content/public/common/content_switches.h (right): https://codereview.chromium.org/2141793002/diff/1/content/public/common/conte... content/public/common/content_switches.h:113: CONTENT_EXPORT extern const char kEnableCanvas2dDynamicPipelineMode[]; Pipeline -> Rendering https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:328: if (m_context->is2d() && buffer()->isAccelerated() && m_numFramesSincePipelineSwitch >= 3) { This hard coded 3 should go into a named constant in ExpensiveCanvasHeuristicParameters https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.h:266: int m_numFramesSincePipelineSwitch; m_numFramesSinceLastRenderingModeSwitch; https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:112: virtual bool shouldAccelerate() const { return true; } There are already is a method called "shouldAccelerate" on HTMLCAnvasElement, that makes a decision based on other parameter. This one should be called something else to avoid confusion. Perhaps isAccelerationOptimalForCanvasContent() https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1089: numDrawPathCalls * 0.004 + These coefficients should all go in ExpensiveCAnvasHeuristicParameters, along with comment that explaining how they were determined and any caveats. https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1098: double unacceleratedPenalty = 1.5; // greater than 1 to create a bias towards suggesting acceleration This threshold should be in ExpensiveCanvasHeuristicParameters https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:834: EXPECT_EQ(true, canvasElement().buffer()->isAccelerated()); use EXPECT_TRUE https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:837: EXPECT_EQ(false, canvasElement().buffer()->isAccelerated()); use EXPECT_FALSE https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:39: EnableCanvas2dDynamicPipelineMode status=experimental Pipeline -> Rendering https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebRuntimeFeatures.cpp (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebRuntimeFeatures.cpp:108: void WebRuntimeFeatures::enableCanvas2dDynamicPipelineMode(bool enable) Pipeline -> Rendering
Need to specify a bug in the description
Description was changed from ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode flag is enabled. BUG= ========== to ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode flag is enabled. BUG=606687, 606686, 606685 ==========
https://codereview.chromium.org/2141793002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2141793002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5441: + <message name="IDS_FLAGS_ENABLE_2D_CANVAS_DYNAMIC_PIPELINE_MODE_NAME" desc="Enable canvas 2D dynamic pipeline switching mode."> On 2016/07/12 19:31:23, Justin Novosad wrote: > name -> IDS_FLAGS_ENABLE_2D_CANVAS_DYNAMIC_RENDERING_MODE_NAME > desc -> Enable 2D canvas dynamic rendering mode switching. Done. https://codereview.chromium.org/2141793002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5444: + <message name="IDS_FLAGS_ENABLE_2D_CANVAS_DYNAMIC_PIPELINE_MODE_DESCRIPTION" desc="Description of 'Enable canvas 2D dynamic pipeline switching mode'"> On 2016/07/12 19:31:23, Justin Novosad wrote: > Same here. Done. https://codereview.chromium.org/2141793002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5445: + Enables dynamic graphics rendering pipeline switching in the 2D canvas to increase performance. On 2016/07/12 19:31:23, Justin Novosad wrote: > (...) to optimize performance based on the types of draw operations being > invoked. Done. https://codereview.chromium.org/2141793002/diff/1/content/public/common/conte... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2141793002/diff/1/content/public/common/conte... content/public/common/content_switches.cc:367: "enable-canvas-2d-dynamic-pipeline-mode"; On 2016/07/12 19:31:23, Justin Novosad wrote: > pipeline->rendering > Pipeline is a loaded word that could easily be misinterpred in this case, let's > call this a rendering mode. Done. https://codereview.chromium.org/2141793002/diff/1/content/public/common/conte... File content/public/common/content_switches.h (right): https://codereview.chromium.org/2141793002/diff/1/content/public/common/conte... content/public/common/content_switches.h:113: CONTENT_EXPORT extern const char kEnableCanvas2dDynamicPipelineMode[]; On 2016/07/12 19:31:23, Justin Novosad wrote: > Pipeline -> Rendering Done. https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:328: if (m_context->is2d() && buffer()->isAccelerated() && m_numFramesSincePipelineSwitch >= 3) { On 2016/07/12 19:31:23, Justin Novosad wrote: > This hard coded 3 should go into a named constant in > ExpensiveCanvasHeuristicParameters Done. https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLCanvasElement.h:266: int m_numFramesSincePipelineSwitch; On 2016/07/12 19:31:23, Justin Novosad wrote: > m_numFramesSinceLastRenderingModeSwitch; Done. https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:112: virtual bool shouldAccelerate() const { return true; } On 2016/07/12 19:31:24, Justin Novosad wrote: > There are already is a method called "shouldAccelerate" on HTMLCAnvasElement, > that makes a decision based on other parameter. This one should be called > something else to avoid confusion. Perhaps > isAccelerationOptimalForCanvasContent() Done. https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1089: numDrawPathCalls * 0.004 + On 2016/07/12 19:31:24, Justin Novosad wrote: > These coefficients should all go in ExpensiveCAnvasHeuristicParameters, along > with comment that explaining how they were determined and any caveats. Done. https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1098: double unacceleratedPenalty = 1.5; // greater than 1 to create a bias towards suggesting acceleration On 2016/07/12 19:31:24, Justin Novosad wrote: > This threshold should be in ExpensiveCanvasHeuristicParameters Done. https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:834: EXPECT_EQ(true, canvasElement().buffer()->isAccelerated()); On 2016/07/12 19:31:24, Justin Novosad wrote: > use EXPECT_TRUE Done. https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:837: EXPECT_EQ(false, canvasElement().buffer()->isAccelerated()); On 2016/07/12 19:31:24, Justin Novosad wrote: > use EXPECT_FALSE Done. https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:39: EnableCanvas2dDynamicPipelineMode status=experimental On 2016/07/12 19:31:24, Justin Novosad wrote: > Pipeline -> Rendering Done. https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebRuntimeFeatures.cpp (right): https://codereview.chromium.org/2141793002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebRuntimeFeatures.cpp:108: void WebRuntimeFeatures::enableCanvas2dDynamicPipelineMode(bool enable) On 2016/07/12 19:31:24, Justin Novosad wrote: > Pipeline -> Rendering Done.
Need to add owners content/ and chrome/ https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:332: Platform::current()->currentThread()->getWebTaskRunner()->postTask( Add a comment to explain why the switch is asynchronous (can't switch during paint invalidation step) https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h (right): https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:191: virtual const UsageCounters& getUsage(); Why was this made virtual?
https://codereview.chromium.org/2141793002/diff/20001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2141793002/diff/20001/content/public/common/c... content/public/common/content_switches.cc:367: "enable-canvas-2d-dynamic-rendering-mode"; As discussed, please add "switching" to these flags and enums. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:333: BLINK_FROM_HERE, WTF::bind(&disableAccelerationWrapper, m_imageBuffer->m_weakPtrFactory.createWeakPtr())); Out of curiosity, would it be possible to use a lambda here? disableAccelerationWrapper() seems simple enough that it would be nice to put its contents inline. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h (right): https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h:88: const double AcceleratedDrawPathApproximateCost = 0.004; Could you put the methodology you used to create the constants into a doc, so that we can re-run them as the rendering algorithms change? In particular, I plan to change the path rendering algorithm which should should bump a lot of these numbers. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h:92: const double RecordingDrawPathApproximateCost = 0.0014; Bikeshedding on names a bit: I'm not sure I like the nomenclature of "Accelerated" vs "Recording". Isn't this really accelerated vs non-accelerated, it's just that the latter uses the display list path, which is (currently) not accelerated? And what about the devices on which the recording path *is* accelerated? (I thought this was the case on Android, but I could be wrong.) Should we use different constants there? https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h:93: const double RecordingGetImageDataApproximateCost = 0.001; I'm a little confused about GetImageData in "Recording" mode. My understand was that this isn't possible (since we'd need to hand pixels back from the compositor thread to the main thread), and we drop to non-display-list mode already (prior to your change). Justin, could you clarify? Are we really ending up in software-renderered, display-list mode here on a GetImageData? If not, this "Recording" name is probably wrong. Maybe we should just add a comment here to explain that this "Recording" mode may not actually be recording. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:405: class RecordingSurfaceFactory : public RecordingImageBufferFallbackSurfaceFactory { As discussed, rename to UnacceleratedSurfaceFactory?
sebastienlc@google.com changed reviewers: + jochen@chromium.org, thakis@chromium.org
The CQ bit was checked by sebastienlc@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2141793002/diff/20001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2141793002/diff/20001/content/public/common/c... content/public/common/content_switches.cc:367: "enable-canvas-2d-dynamic-rendering-mode"; On 2016/07/14 18:53:04, Stephen White wrote: > As discussed, please add "switching" to these flags and enums. Done. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:332: Platform::current()->currentThread()->getWebTaskRunner()->postTask( On 2016/07/14 18:07:10, Justin Novosad wrote: > Add a comment to explain why the switch is asynchronous (can't switch during > paint invalidation step) Done. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:333: BLINK_FROM_HERE, WTF::bind(&disableAccelerationWrapper, m_imageBuffer->m_weakPtrFactory.createWeakPtr())); On 2016/07/14 18:53:04, Stephen White wrote: > Out of curiosity, would it be possible to use a lambda here? > disableAccelerationWrapper() seems simple enough that it would be nice to put > its contents inline. Done. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h (right): https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:191: virtual const UsageCounters& getUsage(); On 2016/07/14 18:07:10, Justin Novosad wrote: > Why was this made virtual? Done. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h (right): https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h:88: const double AcceleratedDrawPathApproximateCost = 0.004; On 2016/07/14 18:53:05, Stephen White wrote: > Could you put the methodology you used to create the constants into a doc, so > that we can re-run them as the rendering algorithms change? In particular, I > plan to change the path rendering algorithm which should should bump a lot of > these numbers. Done. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h:92: const double RecordingDrawPathApproximateCost = 0.0014; On 2016/07/14 18:53:05, Stephen White wrote: > Bikeshedding on names a bit: I'm not sure I like the nomenclature of > "Accelerated" vs "Recording". Isn't this really accelerated vs non-accelerated, > it's just that the latter uses the display list path, which is (currently) not > accelerated? > > And what about the devices on which the recording path *is* accelerated? (I > thought this was the case on Android, but I could be wrong.) Should we use > different constants there? Done. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h:93: const double RecordingGetImageDataApproximateCost = 0.001; On 2016/07/14 18:53:04, Stephen White wrote: > I'm a little confused about GetImageData in "Recording" mode. My understand was > that this isn't possible (since we'd need to hand pixels back from the > compositor thread to the main thread), and we drop to non-display-list mode > already (prior to your change). Justin, could you clarify? Are we really ending > up in software-renderered, display-list mode here on a GetImageData? If not, > this "Recording" name is probably wrong. > > Maybe we should just add a comment here to explain that this "Recording" mode > may not actually be recording. Done. https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/2141793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:405: class RecordingSurfaceFactory : public RecordingImageBufferFallbackSurfaceFactory { On 2016/07/14 18:53:05, Stephen White wrote: > As discussed, rename to UnacceleratedSurfaceFactory? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sebastienlc@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sebastienlc@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
(I'm waiting for junov/senorblanco to finish their reviews)
The CQ bit was checked by sebastienlc@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sebastienlc@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm with documentation nit. https://codereview.chromium.org/2141793002/diff/90021/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h (right): https://codereview.chromium.org/2141793002/diff/90021/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h:88: // https://docs.google.com/a/google.com/document/d/11pfwfyTb6cxLiNbhpVzO5Bx_s9_d... Not ideal to refer to a google doc here. Would be better to put the documentation in the chromium repository (a markdown file, perhaps?)
The CQ bit was checked by sebastienlc@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sebastienlc@google.com changed reviewers: + jwd@chromium.org
https://codereview.chromium.org/2141793002/diff/90021/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h (right): https://codereview.chromium.org/2141793002/diff/90021/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ExpensiveCanvasHeuristicParameters.h:88: // https://docs.google.com/a/google.com/document/d/11pfwfyTb6cxLiNbhpVzO5Bx_s9_d... On 2016/07/18 14:22:43, Justin Novosad wrote: > Not ideal to refer to a google doc here. Would be better to put the > documentation in the chromium repository (a markdown file, perhaps?) Done. The way that the coefficients are estimated will change soon. I'll document the coefficient estimation methodology in more details in a separate document in the chromium repository after the change.
Description was changed from ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode flag is enabled. BUG=606687, 606686, 606685 ========== to ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode-switching flag is enabled. BUG=606687, 606686, 606685 ==========
Histograms LGTM. Are there any plans/methods for determining the effectiveness of the heuristic and different pipeline in the wild?
On 2016/07/19 14:48:43, jwd wrote: > Histograms LGTM. > > Are there any plans/methods for determining the effectiveness of the heuristic > and different pipeline in the wild? Yes, we were just discussing this earlier. We are going to do two things: 1) Add a histogram bin for this fallback mechanism to the existing histogram CanvasContextUsage 2) Create a new histogram to track mode switch reasons
https://codereview.chromium.org/2141793002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2141793002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:324: if (RuntimeEnabledFeatures::enableCanvas2dDynamicRenderingModeSwitchingEnabled()) { Nit: perhaps this check should be outside of all of this code (ie., above line 321 above). Would that make sense? https://codereview.chromium.org/2141793002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:329: WTF::bind([](WeakPtr<ImageBuffer> buffer) { Do you still need WTF::bind() here, or could you just use the lambda directly? The type conversion *should* just work without the bind(), although I'm half-speculating here. Maybe give it a try?
On 2016/07/19 14:48:43, jwd wrote: > Histograms LGTM. > > Are there any plans/methods for determining the effectiveness of the heuristic > and different pipeline in the wild? Yes, we're planning on adding histogram metrics in the future.
https://codereview.chromium.org/2141793002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2141793002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:324: if (RuntimeEnabledFeatures::enableCanvas2dDynamicRenderingModeSwitchingEnabled()) { On 2016/07/19 15:47:41, Stephen White wrote: > Nit: perhaps this check should be outside of all of this code (ie., above line > 321 above). Would that make sense? Done. https://codereview.chromium.org/2141793002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:329: WTF::bind([](WeakPtr<ImageBuffer> buffer) { On 2016/07/19 15:47:41, Stephen White wrote: > Do you still need WTF::bind() here, or could you just use the lambda directly? > The type conversion *should* just work without the bind(), although I'm > half-speculating here. Maybe give it a try? I tried but the type conversion doesn't seem to work.
On 2016/07/19 15:45:35, Justin Novosad wrote: > On 2016/07/19 14:48:43, jwd wrote: > > Histograms LGTM. > > > > Are there any plans/methods for determining the effectiveness of the heuristic > > and different pipeline in the wild? > > Yes, we were just discussing this earlier. We are going to do two things: > 1) Add a histogram bin for this fallback mechanism to the existing histogram > CanvasContextUsage What do you mean by histogram bin? As in new versions of existing histograms that will let you see performance and such of the fallback mechanism? > 2) Create a new histogram to track mode switch reasons
On 2016/07/19 15:45:35, Justin Novosad wrote: > On 2016/07/19 14:48:43, jwd wrote: > > Histograms LGTM. > > > > Are there any plans/methods for determining the effectiveness of the heuristic > > and different pipeline in the wild? > > Yes, we were just discussing this earlier. We are going to do two things: > 1) Add a histogram bin for this fallback mechanism to the existing histogram > CanvasContextUsage What do you mean by histogram bin? As in new versions of existing histograms that will let you see performance and such of the fallback mechanism? > 2) Create a new histogram to track mode switch reasons
LGTM
lgtm
Description was changed from ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode-switching flag is enabled. BUG=606687, 606686, 606685 ========== to ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode-switching flag is enabled. BUG=606687, 606686, 606685 TBR=grt@chromium.org ==========
The CQ bit was checked by junov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2141793002/#ps130001 (title: "Minor change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode-switching flag is enabled. BUG=606687, 606686, 606685 TBR=grt@chromium.org ========== to ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode-switching flag is enabled. BUG=606687, 606686, 606685 TBR=grt@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode-switching flag is enabled. BUG=606687, 606686, 606685 TBR=grt@chromium.org ========== to ========== Improving canvas 2D performance by switching graphics rendering pipeline. 1. Functionality to switch from the accelerated (GPU) graphics pipeline to the recording graphics pipeline. 2. Simple heuristic to determine when the pipeline switch would increase performance based on how the canvas was used previously. 3. Automatically switch pipeline to increase performance based on the heuristic if the enable-canvas-2d-dynamic-pipeline-mode-switching flag is enabled. BUG=606687, 606686, 606685 TBR=grt@chromium.org Committed: https://crrev.com/98739b054a5b28cc607d688b2e9e582456bbb20e Cr-Commit-Position: refs/heads/master@{#406585} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/98739b054a5b28cc607d688b2e9e582456bbb20e Cr-Commit-Position: refs/heads/master@{#406585} |
