Question about painting code and CullRect

[Manuel Rego Casasnovas]

Hi,

In Grid Layout we have a optimization in the painting code since many
years ago.
Basically it avoids to loop all the children (grid items) of a grid
container in order to paint it. Instead if check which columns and rows
are "visible" and paint only the items in those (there are corner cases
and things like that but this is the main idea). That way we avoid to
loop all the grid items on painting and we only process the ones that
are visible.

This optimization is implemented in GridPainter::PaintChildren() [1],
where we use paint_info.GetCullRect().Rect() to check what we should paint.
However we have realized that lately (we don't know since when it
happens) this CullRect is really big.
For example in a 800x600 window, if the grid is bigger than that like
1000x1000 we get that size as the CullRect. If we have a grid of
2000x2000, again we get 2000x2000. Only if we have something huge like
5000x5000 (or bigger) then we get 4800x4600.

With such big numbers the grid specific optimization doesn't make sense
anymore, as we're having a more complex code and likely painting the
whole grid in most cases.
BTW, this is not noticeable on the perftests as they measure layout time
and not painting, so we didn't get any regression because of this.

So the question is if anyone know when this was modified or why we're
seeing such big CullRects?
And if this is the expected behavior for that, probably we should just
remove the Grid Layout optimization to simplify the code.

What do you think?

Thanks,
Rego

[1]
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/grid_painter.cc?l=47

[Philip Rogers]

Blink paints far outside the viewport for composited scrolling which lets the user scroll entirely on the compositor thread without going back to blink to paint. This is called the "interest rect" (see: CompositedLayerMapping::RecomputeInterestRect) and I think it is ending up as the cull rect in the grid painting code. Just looking briefly, I think the grid_painter logic that skips culled children may be redundant with the early-out in the block painting code used by the children.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/407a210a-cb90-7817-da48-62b532fb821e%40igalia.com.

[Christian Biesinger]

FWIW, there is code in perf_tests/paint which I would assume does test
painting performance.

[Manuel Rego Casasnovas]

On 19/09/18 17:59, Philip Rogers wrote:
> Blink paints far outside the viewport for composited scrolling which
> lets the user scroll entirely on the compositor thread without going
> back to blink to paint. This is called the "interest rect" (see:
> CompositedLayerMapping::RecomputeInterestRect) and I think it is ending
> up as the cull rect in the grid painting code. Just looking briefly, I
> think the grid_painter logic that skips culled children may be redundant
> with the early-out in the block painting code used by the children.

The idea of the optimization was avoiding to loop all the children and
just the ones visible.
If you a big grid of 500x500 (500 columns and 500 rows) and one item per
cell, but only 100x100 are visible. You would call the painting code for
10,000 items instead of calling it for 250,000.

But with the current CullRects the optimization would only make sense
when the grid is really big (bigger than 4800x4600 pixels), so it's not
useful at all anymore (only in some corner cases) and it's making things
worse in some scenarios where the grid is smaller than that size.

Thanks for the info.

Bye,
Rego

> To unsubscribe from this group and stop receiving emails from it, send
> an email to blink-dev+...@chromium.org
> <mailto:blink-dev+...@chromium.org>.

> To view this discussion on the web visit

> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAJgFLLta4jZWXLcVGfSdxGHQXsyg6fV_Ja6x-qNnUUHXnvwF9Q%40mail.gmail.com
> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAJgFLLta4jZWXLcVGfSdxGHQXsyg6fV_Ja6x-qNnUUHXnvwF9Q%40mail.gmail.com?utm_medium=email&utm_source=footer>.

[Manuel Rego Casasnovas]

On 20/09/18 01:52, Christian Biesinger wrote:
> FWIW, there is code in perf_tests/paint which I would assume does test
> painting performance.

But we don't have any grid layout test there, dunno why we didn't add
one when the optimization was originally implemented (but that was like
4 years ago) but we didn't catch any regression as we lacked tests.

Anyway doing some quick tests there I verified that the "optimization"
is actually slowing down some cases so we'll go ahead and get rid of it
simplifying the code.

Thanks,
Rego

[Chris Harrelson]

On Thu, Sep 20, 2018 at 2:30 AM Manuel Rego Casasnovas <re...@igalia.com> wrote:

On 20/09/18 01:52, Christian Biesinger wrote:
> FWIW, there is code in perf_tests/paint which I would assume does test
> painting performance.

But we don't have any grid layout test there, dunno why we didn't add
one when the optimization was originally implemented (but that was like
4 years ago) but we didn't catch any regression as we lacked tests.

Anyway doing some quick tests there I verified that the "optimization"
is actually slowing down some cases so we'll go ahead and get rid of it
simplifying the code.

That sounds like the right decision to me. Thanks for digging into the root cause of this complexity and simplifying the code.

 

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/882536b7-2787-b363-a8a2-16ad118effb5%40igalia.com.