Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1898813002:
Allow throttling in painting microbenchmark (Closed)
|
Created:
4 years, 8 months ago by Sami Modified:
4 years, 7 months ago CC:
blink-layers+watch_chromium.org, blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dglazkov+blink, dshwang, drott+blinkwatch_chromium.org, krit, eae+blinkwatch, f(malita), jbroman, jchaffraix+rendering, Justin Novosad, kinuko+watch, leviw+renderwatch, pdr+graphicswatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, sof, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow throttling in painting microbenchmark
ContentLayerDelegate::paintContents() is being used as an alternate
painting path by the rasterize and record microbenchmark. This benchmark
can crash if any frame on the page is being throttled, because this
way of painting doesn't enable throttling. This patch fixes that by
enabling throttling, which also makes the benchmark more realistic since
the real painting path also uses throttling.
BUG=600377
TEST=tools/perf/run_benchmark rasterize_and_record_micro.top_25_smooth --browser=debug --story-filter=Blogger
Committed: https://crrev.com/30585301cd73bf1e6cff2c9bf22422e515b728d8
Cr-Commit-Position: refs/heads/master@{#388598}
Patch Set 1 #Patch Set 2 : Added test. #Patch Set 3 : Always throttle in CompositedLayerMapping #Patch Set 4 : Rebased #
Total comments: 2
Messages
Total messages: 37 (11 generated)
Description was changed from ========== Allow throttling in painting microbenchmark ContentLayerDelegate::paintContents() is being used as an alternate painting path by the rasterize and record microbenchmark. This benchmark can crash if any frame on the page is being throttled, because this way of painting doesn't enable throttling. This patch fixes that by enabling throttling, which also makes the benchmark more realistic since the real painting path also uses throttling. BUG=600377 TEST=tools/perf/run_benchmark rasterize_and_record_micro.top_25_smooth --browser=debug --story-filter=Blogger ========== to ========== Allow throttling in painting microbenchmark ContentLayerDelegate::paintContents() is being used as an alternate painting path by the rasterize and record microbenchmark. This benchmark can crash if any frame on the page is being throttled, because this way of painting doesn't enable throttling. This patch fixes that by enabling throttling, which also makes the benchmark more realistic since the real painting path also uses throttling. BUG=600377 TEST=tools/perf/run_benchmark rasterize_and_record_micro.top_25_smooth --browser=debug --story-filter=Blogger ==========
skyostil@chromium.org changed reviewers: + wangxianzhu@chromium.org
Something along these lines? No unit test yet, and I'm not sure what to do about the dependency to base::WrapUnique.
In what cases that we should not allow throttling?
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org
+chrishtr@ for more opinions.
I was thinking about the same thing. The only case that comes to mind is printing or otherwise capturing the entire page. I'm not entirely sure how that relates to painting though.
On 2016/04/19 09:59:59, Sami wrote: > I was thinking about the same thing. The only case that comes to mind is > printing or otherwise capturing the entire page. I'm not entirely sure how that > relates to painting though. I checked that the entrypoint for printing is FrameView::paintContents(), so it seems like we can't just always enable throttling inside the painting stack.
On 2016/04/19 10:09:42, Sami wrote: > On 2016/04/19 09:59:59, Sami wrote: > > I was thinking about the same thing. The only case that comes to mind is > > printing or otherwise capturing the entire page. I'm not entirely sure how > that > > relates to painting though. > > I checked that the entrypoint for printing is FrameView::paintContents(), so it > seems like we can't just always enable throttling inside the painting stack. I'm thinking of enabling throttling in CompositedLayerMapping::paintContents() (as your proposal 1 in crbug.com/600377#c34), to avoid exposing throttling from core to platform. Is there any entrypoint of CompositedLayerMapping::paintContents() expecting no-throttling?
On 2016/04/19 16:51:13, Xianzhu wrote: > On 2016/04/19 10:09:42, Sami wrote: > > On 2016/04/19 09:59:59, Sami wrote: > > > I was thinking about the same thing. The only case that comes to mind is > > > printing or otherwise capturing the entire page. I'm not entirely sure how > > that > > > relates to painting though. > > > > I checked that the entrypoint for printing is FrameView::paintContents(), so > it > > seems like we can't just always enable throttling inside the painting stack. > > I'm thinking of enabling throttling in CompositedLayerMapping::paintContents() > (as your proposal 1 in crbug.com/600377#c34), to avoid exposing throttling from > core to platform. Is there any entrypoint of > CompositedLayerMapping::paintContents() expecting no-throttling? Doesn't FrameView::paintContents() end up going there? If not, then throttling there would certainly be simpler.
On 2016/04/19 16:56:33, Sami wrote: > On 2016/04/19 16:51:13, Xianzhu wrote: > > On 2016/04/19 10:09:42, Sami wrote: > > > On 2016/04/19 09:59:59, Sami wrote: > > > > I was thinking about the same thing. The only case that comes to mind is > > > > printing or otherwise capturing the entire page. I'm not entirely sure how > > > that > > > > relates to painting though. > > > > > > I checked that the entrypoint for printing is FrameView::paintContents(), so > > it > > > seems like we can't just always enable throttling inside the painting stack. > > > > I'm thinking of enabling throttling in CompositedLayerMapping::paintContents() > > (as your proposal 1 in crbug.com/600377#c34), to avoid exposing throttling > from > > core to platform. Is there any entrypoint of > > CompositedLayerMapping::paintContents() expecting no-throttling? > > Doesn't FrameView::paintContents() end up going there? If not, then throttling > there would certainly be simpler. FrameView::paintContents() won't go to CompositedLayerMapping::paintContents(). During printing, PaintLayerPainter will get GlobalPaintFlattenCompositingLayers flag to paint all composited layers.
On 2016/04/19 17:42:19, Xianzhu wrote: > FrameView::paintContents() won't go to CompositedLayerMapping::paintContents(). > During printing, PaintLayerPainter will get GlobalPaintFlattenCompositingLayers > flag to paint all composited layers. Thanks for confirming. I've now made CompositedLayerMapping always enforce throttling.
The CQ bit was checked by wangxianzhu@chromium.org
lgtm
The CQ bit was checked by wangxianzhu@chromium.org
lgtm lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1898813002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1898813002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wangxianzhu@chromium.org Link to the patchset: https://codereview.chromium.org/1898813002/#ps60001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1898813002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1898813002/60001
Message was sent while issue was closed.
Description was changed from ========== Allow throttling in painting microbenchmark ContentLayerDelegate::paintContents() is being used as an alternate painting path by the rasterize and record microbenchmark. This benchmark can crash if any frame on the page is being throttled, because this way of painting doesn't enable throttling. This patch fixes that by enabling throttling, which also makes the benchmark more realistic since the real painting path also uses throttling. BUG=600377 TEST=tools/perf/run_benchmark rasterize_and_record_micro.top_25_smooth --browser=debug --story-filter=Blogger ========== to ========== Allow throttling in painting microbenchmark ContentLayerDelegate::paintContents() is being used as an alternate painting path by the rasterize and record microbenchmark. This benchmark can crash if any frame on the page is being throttled, because this way of painting doesn't enable throttling. This patch fixes that by enabling throttling, which also makes the benchmark more realistic since the real painting path also uses throttling. BUG=600377 TEST=tools/perf/run_benchmark rasterize_and_record_micro.top_25_smooth --browser=debug --story-filter=Blogger ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Allow throttling in painting microbenchmark ContentLayerDelegate::paintContents() is being used as an alternate painting path by the rasterize and record microbenchmark. This benchmark can crash if any frame on the page is being throttled, because this way of painting doesn't enable throttling. This patch fixes that by enabling throttling, which also makes the benchmark more realistic since the real painting path also uses throttling. BUG=600377 TEST=tools/perf/run_benchmark rasterize_and_record_micro.top_25_smooth --browser=debug --story-filter=Blogger ========== to ========== Allow throttling in painting microbenchmark ContentLayerDelegate::paintContents() is being used as an alternate painting path by the rasterize and record microbenchmark. This benchmark can crash if any frame on the page is being throttled, because this way of painting doesn't enable throttling. This patch fixes that by enabling throttling, which also makes the benchmark more realistic since the real painting path also uses throttling. BUG=600377 TEST=tools/perf/run_benchmark rasterize_and_record_micro.top_25_smooth --browser=debug --story-filter=Blogger Committed: https://crrev.com/30585301cd73bf1e6cff2c9bf22422e515b728d8 Cr-Commit-Position: refs/heads/master@{#388598} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/30585301cd73bf1e6cff2c9bf22422e515b728d8 Cr-Commit-Position: refs/heads/master@{#388598}
Message was sent while issue was closed.
ojan@chromium.org changed reviewers: + ojan@chromium.org
Message was sent while issue was closed.
The need for this makes me wonder if we should change rasterize and record benchmark. If real web pages can't hit this, then the benchmark isn't testing a real codepath. We're taking on some technical debt with the current render pipeline throttling that we'll need to pay off at some point, i.e. eventually we shouldn't have throttling-related things anywhere except for updateAllLifeCyclePhases. I think that's the only way to avoid having bugs with throttling as we refactor code.
Message was sent while issue was closed.
https://codereview.chromium.org/1898813002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right): https://codereview.chromium.org/1898813002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:677: paintRecursively(layer, &displayItems); Specifically, this is not a thing that can happen on the web, right? (painting outside of a lifecycle update)
Message was sent while issue was closed.
https://codereview.chromium.org/1898813002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right): https://codereview.chromium.org/1898813002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:677: paintRecursively(layer, &displayItems); On 2016/04/28 21:47:27, ojan wrote: > Specifically, this is not a thing that can happen on the web, right? (painting > outside of a lifecycle update) Right. However, I think this is why we call rasterize_and_record_micro a micro benchmark: it provides several controlled modes to simulate extreme conditions (e.g. no caching, no subsequence caching but have display item caching, no painting, etc.) to expose micro aspects of painting performance. For painting performance in real scenarios, we can use the UMA performance measurements. We can also collect the UMA performance measurements in a benchmark.
Message was sent while issue was closed.
On 2016/04/28 at 21:56:32, wangxianzhu wrote: > https://codereview.chromium.org/1898813002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right): > > https://codereview.chromium.org/1898813002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:677: paintRecursively(layer, &displayItems); > On 2016/04/28 21:47:27, ojan wrote: > > Specifically, this is not a thing that can happen on the web, right? (painting > > outside of a lifecycle update) > > Right. > > However, I think this is why we call rasterize_and_record_micro a micro benchmark: it provides several controlled modes to simulate extreme conditions (e.g. no caching, no subsequence caching but have display item caching, no painting, etc.) to expose micro aspects of painting performance. I don't think that's the same thing. microbenchmarks should still *only* tests real codepaths that can actually be hit in the product. When it gets to a point that we're adding code changes to the production product just for a microbenchmark that isn't something real code can hit, I think there's something wrong with the benchmark. A micro-benchmark for WTF::Vector would be different. All the codepaths would be ones that are hit in the real product.
Message was sent while issue was closed.
I guess I'm wondering if we could test similar things while still actually running the whole updateAllLifeCyclePhases instead of calling into paint directly. Enne, you worked on this benchmark in the past. Do you have any ideas how this could be improved?
Message was sent while issue was closed.
On 2016/04/28 22:29:23, ojan wrote: > On 2016/04/28 at 21:56:32, wangxianzhu wrote: > > > https://codereview.chromium.org/1898813002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right): > > > > > https://codereview.chromium.org/1898813002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:677: > paintRecursively(layer, &displayItems); > > On 2016/04/28 21:47:27, ojan wrote: > > > Specifically, this is not a thing that can happen on the web, right? > (painting > > > outside of a lifecycle update) > > > > Right. > > > > However, I think this is why we call rasterize_and_record_micro a micro > benchmark: it provides several controlled modes to simulate extreme conditions > (e.g. no caching, no subsequence caching but have display item caching, no > painting, etc.) to expose micro aspects of painting performance. > > I don't think that's the same thing. microbenchmarks should still *only* tests > real codepaths that can actually be hit in the product. When it gets to a point > that we're adding code changes to the production product just for a > microbenchmark that isn't something real code can hit, I think there's something > wrong with the benchmark. > > A micro-benchmark for WTF::Vector would be different. All the codepaths would be > ones that are hit in the real product. The rasterize_and_record_micro benchmark is testing whole swaths of real code paths, just via an entry point that is not used elsewhere. The benchmark could presumably be somehow modified to get the same coverage while using only production entry points, but that's a relatively big job and I strongly suspect it would require a different set of modifications to production code. Specifically, the parameters that control the benchmark would need to get mapped through somehow. UMA data can give us the real world perf numbers that we need. I'm far less concerned right now with this kind of testing than I am with the fact that we failed to catch major degradations to animated image rendering quality. If we're putting resources into improved testing, visual quality related metrics seem more important right now than getting better performance testing.
Message was sent while issue was closed.
> I don't think that's the same thing. microbenchmarks should still *only* tests real codepaths that can actually be hit in the product. When it gets to a point that we're adding code changes to the production product just for a microbenchmark that isn't something real code can hit, I think there's something wrong with the benchmark. I'm sorry, but I think I disagree a little bit with this. The bits that are different are the scaffolding ones that won't show up in a performance trace, and it's the bits lower down in the pipeline that are the important things to measure and are shared in both. If this microbenchmark replaced out BoxPainter, I'd say that it wasn't testing real code. I think this is different. I do agree that there is a complexity cost to maintaining these extra codepaths. It's awkward to have to hack these things in and to think about the ways in which they are narrow, but there's also a lot of value here too. This microbenchmark was written in a time when PictureLayer and PictureLayerImpl drove recording and raster. As slimming paint continues to push recording responsibility and caching up into Blink, we will need a separate recording (and layout and style and and and) microbenchmark in Blink, in my opinion. I think this is what you're suggesting to do and I think we should do it, but I think this is a reasonable band-aid in the short term.
Message was sent while issue was closed.
On 2016/04/29 at 18:19:09, enne wrote: > > I don't think that's the same thing. microbenchmarks should still *only* tests real codepaths that can actually be hit in the product. When it gets to a point that we're adding code changes to the production product just for a microbenchmark that isn't something real code can hit, I think there's something wrong with the benchmark. > > I'm sorry, but I think I disagree a little bit with this. The bits that are different are the scaffolding ones that won't show up in a performance trace, and it's the bits lower down in the pipeline that are the important things to measure and are shared in both. If this microbenchmark replaced out BoxPainter, I'd say that it wasn't testing real code. I think this is different. > > I do agree that there is a complexity cost to maintaining these extra codepaths. It's awkward to have to hack these things in and to think about the ways in which they are narrow, but there's also a lot of value here too. This microbenchmark was written in a time when PictureLayer and PictureLayerImpl drove recording and raster. As slimming paint continues to push recording responsibility and caching up into Blink, we will need a separate recording (and layout and style and and and) microbenchmark in Blink, in my opinion. I think this is what you're suggesting to do and I think we should do it, but I think this is a reasonable band-aid in the short term. I'm not suggesting that we roll back this patch. But I'm worried that we allow throttling in this case for a benchmark and that we'll accidentally start throttling in places we shouldn't in the shipping product and that we won't notice because of this code. Incidentally, we now also have a test that isn't tied to the benchmark it's there for. I'd feel better if there was a plan to fix this (eventually) and a corresponding TODO in the code to remove this line + the test. I'm not suggesting that actually fixing it needs to be a P0 we drop other work for though.
Message was sent while issue was closed.
On 2016/04/29 at 14:34:00, schenney wrote: > On 2016/04/28 22:29:23, ojan wrote: > > On 2016/04/28 at 21:56:32, wangxianzhu wrote: > > > > > https://codereview.chromium.org/1898813002/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right): > > > > > > > > https://codereview.chromium.org/1898813002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:677: > > paintRecursively(layer, &displayItems); > > > On 2016/04/28 21:47:27, ojan wrote: > > > > Specifically, this is not a thing that can happen on the web, right? > > (painting > > > > outside of a lifecycle update) > > > > > > Right. > > > > > > However, I think this is why we call rasterize_and_record_micro a micro > > benchmark: it provides several controlled modes to simulate extreme conditions > > (e.g. no caching, no subsequence caching but have display item caching, no > > painting, etc.) to expose micro aspects of painting performance. > > > > I don't think that's the same thing. microbenchmarks should still *only* tests > > real codepaths that can actually be hit in the product. When it gets to a point > > that we're adding code changes to the production product just for a > > microbenchmark that isn't something real code can hit, I think there's something > > wrong with the benchmark. > > > > A micro-benchmark for WTF::Vector would be different. All the codepaths would be > > ones that are hit in the real product. > > The rasterize_and_record_micro benchmark is testing whole swaths of real code paths, just via an entry point that is not used elsewhere. The benchmark could presumably be somehow modified to get the same coverage while using only production entry points, but that's a relatively big job and I strongly suspect it would require a different set of modifications to production code. Specifically, the parameters that control the benchmark would need to get mapped through somehow. Maybe I'm misunderstanding something then. My understanding is that this AllowThrottlingScope is only needed for the benchmark and can't be hit on real web content. If that's the case, then it's not *just* a different entry point. It's actually exercising the code in a way that's not possible on real content. > UMA data can give us the real world perf numbers that we need. I'm far less concerned right now with this kind of testing than I am with the fact that we failed to catch major degradations to animated image rendering quality. If we're putting resources into improved testing, visual quality related metrics seem more important right now than getting better performance testing. Hopefully my other request below makes it more clear that I'm not asking that we scramble to fix this now, but rather have a plan for eventually doing so and a TODO in the code to track it.
Message was sent while issue was closed.
Enne explained to me that this code and need for the throttling exception will go away entirely when SPv2 completes. That entirely addresses my concern that we're taking on technical debt without a plan for paying it off. In general, I think when we're adding technical debt, the minimum bar is that we have a plan for eventually removing it and encode that in a TODO. That doesn't mean we need to do the TODO right away if it genuinely isn't the right time for it. But, in this case, since the code is going to get deleted, I think it's fine to leave out the TODO. :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
