Bug 101656 – Coordinated Graphics: Remove a backing store of GraphicsLayer when the layer is far from the viewport.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

TiledBackingStore computes cover and keep rects to create, keep or remove tiles smartly, but currently TiledBackingStore expects a contents rect is big enough to cover the visibleRect. However, when CoordinatedGraphicsLayer uses TBS, it is usually wrong expectation.

We must compute cover and keep rects using the visibleRect, not intersected visibleRect with contentsRect, because TBS can be used as a backing store of GraphicsLayer.

[Dongseong Hwang]

Created attachment 173135 [details]
Patch

[Dongseong Hwang]

Comment on attachment 173135 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173135&action=review

> Source/WebCore/ChangeLog:31
> +

This representation is fine on terminal using a monospace font

[Kenneth Rohde Christiansen]

Comment on attachment 173135 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173135&action=review

Cool you found this!

> Source/WebCore/ChangeLog:40
> +        Changing cache policy is not testable in layout tests.

Could we find another way of testing it? unit tests?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:278
> +     * We must create or keep the tiles on the HERE region.

s/on/in/

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:312
> -            double distance = tileDistance(visibleRect, currentCoordinate);
> +            double distance = tileDistance(intersectedVisibleRect, currentCoordinate);

why not just use m_visibleRect here? ie. why create the separate intersectedVisibleRect ? Where is it intersected?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:345
> +    // If candidateSize is bigger than bounds (i.e. TBS is used as a backing store of GraphicsLayer), we skip the below adjusting logic which expects bounds covers the given rect.

bounds TO cover the ...

[Dongseong Hwang]

Created attachment 173248 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 173248 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173248&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.h:107
> -    IntRect visibleContentsRect() const;
> +    IntRect visibleContentsRectOnBacking() const;
> +    IntRect visibleRectOnBacking() const;
>      IntRect visibleRect() const;

This is becoming too confusing, between what they represents and in what coordinate system.

[Dongseong Hwang]

Comment on attachment 173248 [details]
Patch

I've tested various sites and checked tiles are removed properly.
After this patch, Coordinated Graphics uses pretty less memory.

I explain in detail below.

View in context: https://bugs.webkit.org/attachment.cgi?id=173248&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:244
> +    return coverageRatio(visibleContentsRectOnBacking()) == 1.0f;

The logic is not changed because visibleContentsRectOnBacking is renamed from visibleContentsRect.

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:284
>      m_visibleRect = visibleRect;

m_visibleRect keeps visibleRect(), not visibleRectOnBacking(), because we must remove invisible tiles.

void TiledBackingStore::coverWithTilesIfNeeded(const FloatPoint& trajectoryVector)
{
    IntRect visibleRect = this->visibleRect();
    ...
    if (m_trajectoryVector == normalizedVector && m_visibleRect == visibleRect)
        return;
    ...
    m_visibleRect = visibleRect;

    createTiles();
}

visibleRectOnBacking() returns IntRect() when we scroll the given TBS of GraphicsLayer off the viewport, and then coverWithTilesIfNeeded() early returns.
It means we can have the change to remove tiles. So we must keep visibleRect().

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:295
> +        return;

If we don't return here, createTile() creates one tile because tileCoordinateForPoint(IntRect()) == Tile::Coordinate(0, 0).
It means we execute following "for loop statement" one time.
  Tile::Coordinate topLeft = tileCoordinateForPoint(coverRect.location());
  for (int yCoordinate = topLeft.y(); yCoordinate <= bottomRight.y(); ++yCoordinate) {

So we must early return to save memory.

[Dongseong Hwang]

(In reply to comment #3)
> (From update of attachment 173135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173135&action=review
> 
> Cool you found this!
> 
> Could we find another way of testing it? unit tests?
I think unit test is possible. After adding API to query the count of tile, we can check the count of tiles during scrolling.

> 
> s/on/in/
> 
> why not just use m_visibleRect here? ie. why create the separate intersectedVisibleRect ? Where is it intersected?
The names are changed in the second patch and I explained.
> 

> bounds TO cover the ...

Thanks for review!

We posted simultaneously. I'll update :)

(In reply to comment #5)
> (From update of attachment 173248 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173248&action=review
> 
> > Source/WebCore/platform/graphics/TiledBackingStore.h:107
> > -    IntRect visibleContentsRect() const;
> > +    IntRect visibleContentsRectOnBacking() const;
> > +    IntRect visibleRectOnBacking() const;
> >      IntRect visibleRect() const;
> 
> This is becoming too confusing, between what they represents and in what coordinate system.

I agree. If name contains "Contents", it is on the contents coordinate system. I want to express which to be intersected with m_rect or not. Could you suggest?

[Dongseong Hwang]

Created attachment 173252 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 173252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173252&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:283
>      const IntRect visibleRect = this->visibleRect();

mapFromContents(m_client->tiledBackingStoreVisibleRect()); 
and get rid of visibleRect?

[Kenneth Rohde Christiansen]

Comment on attachment 173252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173252&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:283
>      const IntRect visibleRect = this->visibleRect();

mapFromContents(m_client->tiledBackingStoreVisibleRect()); 
and get rid of visibleRect?

[Dongseong Hwang]

Created attachment 173254 [details]
Patch

[Dongseong Hwang]

Comment on attachment 173254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173254&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.h:107
>      IntRect visibleRect() const;

How about these names? Is it confused yet?





(In reply to comment #3)
> (From update of attachment 173135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173135&action=review
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:312
> > -            double distance = tileDistance(visibleRect, currentCoordinate);
> > +            double distance = tileDistance(intersectedVisibleRect, currentCoordinate);
> 
> why not just use m_visibleRect here? ie. why create the separate intersectedVisibleRect ? Where is it intersected?

you are right! intersectedVisibleRect is more reasonable. now intersectedVisibleRect is boundedVisibleRect.

[Dongseong Hwang]

(In reply to comment #10)
> (From update of attachment 173252 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173252&action=review
> 
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:283
> >      const IntRect visibleRect = this->visibleRect();
> 
> mapFromContents(m_client->tiledBackingStoreVisibleRect()); 
> and get rid of visibleRect?

Ok, I'll get rid of it!

[Kenneth Rohde Christiansen]

yeah fine!

[Dongseong Hwang]

Created attachment 173258 [details]
Patch

[Dongseong Hwang]

Comment on attachment 173258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173258&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.h:105
>      IntRect visibleRect() const;

I removed boundedSomthings instead of visibleRect.

[WebKit Review Bot]

Comment on attachment 173258 [details]
Patch

Clearing flags on attachment: 173258

Committed r134048: <http://trac.webkit.org/changeset/134048>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dongseong Hwang]

(In reply to comment #18)
> All reviewed patches have been landed.  Closing bug.

Thank you for review!