Bug 154985 – Use larger tiles when possible to reduce per-tile painting overhead

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Use larger tiles when possible to reduce per-tile painting overhead

[Simon Fraser (smfr)]

Created attachment 272782 [details]
Patch

[Tim Horton]

Comment on attachment 272782 [details]
Patch

Overall, I think this is awesome. I am a bit worried about pages that jump repeatedly between scrollable and not scrollable (usually not intentionally); perhaps there could be some hysteresis? Or don't change until mayBegin phase? Or something?

[Build Bot]

Comment on attachment 272782 [details]
Patch

Attachment 272782 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/917964

New failing tests:
compositing/tiling/tiled-layer-resize.html
compositing/tiling/transform-origin-tiled.html
pageoverlay/overlay-large-document-scrolled.html
displaylists/replay-skip-clipped-rect.html
compositing/tiling/rotated-tiled-preserve3d-clamped.html
pageoverlay/overlay-large-document.html
compositing/tiling/rotated-tiled-clamped.html

[Build Bot]

Created attachment 272790 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 272782 [details]
Patch

Attachment 272782 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/917966

New failing tests:
compositing/tiling/tiled-layer-resize.html
compositing/tiling/transform-origin-tiled.html
pageoverlay/overlay-large-document-scrolled.html
displaylists/replay-skip-clipped-rect.html
compositing/tiling/rotated-tiled-preserve3d-clamped.html
imported/blink/fast/text/wide-preformatted.html
pageoverlay/overlay-large-document.html
compositing/tiling/rotated-tiled-clamped.html

[Build Bot]

Created attachment 272792 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 272782 [details]
Patch

Attachment 272782 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/917968

New failing tests:
compositing/tiling/tiled-layer-resize.html
compositing/tiling/transform-origin-tiled.html
pageoverlay/overlay-large-document-scrolled.html
displaylists/replay-skip-clipped-rect.html
compositing/tiling/rotated-tiled-preserve3d-clamped.html
pageoverlay/overlay-large-document.html
compositing/tiling/rotated-tiled-clamped.html

[Build Bot]

Created attachment 272797 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Simon Fraser (smfr)]

Created attachment 272810 [details]
Patch

[Simon Fraser (smfr)]

https://trac.webkit.org/r197541

[Simon Fraser (smfr)]

rdar://problem/23635219

[Said Abou-Hallawa]

Comment on attachment 272810 [details]
Patch

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

> Source/WebCore/platform/graphics/EdgeSet.h:32
> +class EdgeSet {

The functionality of this class is a subset of BoxExtent in LengthBox.h. Why do we add a new class? You can change the name of BoxExtent if you want. Actually this is what I tried to do in https://bugs.webkit.org/show_bug.cgi?id=147495 but it's never got reviewed.

[Simon Fraser (smfr)]

(In reply to comment #12)
> Comment on attachment 272810 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272810&action=review
> 
> > Source/WebCore/platform/graphics/EdgeSet.h:32
> > +class EdgeSet {
> 
> The functionality of this class is a subset of BoxExtent in LengthBox.h. Why
> do we add a new class? You can change the name of BoxExtent if you want.
> Actually this is what I tried to do in
> https://bugs.webkit.org/show_bug.cgi?id=147495 but it's never got reviewed.

BoxExtent should be changed to use this template.

[Antti Koivisto]

Comment on attachment 272810 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/TileGrid.cpp:340
> +    m_tileSize = m_controller.tileSize();
> +    if (m_tileSize != m_tileSizeAtLastRevalidate) {
> +        removeAllTiles();
> +        m_tileSizeAtLastRevalidate = m_tileSize;
> +    }

Is m_tileSizeAtLastRevalidate member actually needed? Doesn't this do the same?

auto newTileSize = m_controller.tileSize();
if (newTileSize != m_tileSize) {
    removeAllTiles();
    m_tileSize = newTileSize;
}

[Said Abou-Hallawa]

Comment on attachment 272810 [details]
Patch

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

>>> Source/WebCore/platform/graphics/EdgeSet.h:32
>>> +class EdgeSet {
>> 
>> The functionality of this class is a subset of BoxExtent in LengthBox.h. Why do we add a new class? You can change the name of BoxExtent if you want. Actually this is what I tried to do in https://bugs.webkit.org/show_bug.cgi?id=147495 but it's never got reviewed.
> 
> BoxExtent should be changed to use this template.

BoxExtent has the functionality to set/get a physical side (or edge) given the logical side name, the text direction and the writing mode. EdgeSet does not have such functionality. I think it does not matter which one stays and which one goes away. What it matters is we should not have two classes one of them is a subset of the other.

[Darin Adler]

Every since r197541 landed, 21 tiled drawing tests have been failing on bots like elcapitan-release-tests-wk2, and also locally on my computer and on EWS, with differences like this one:

- (tile size 800 x 600)
- (top left tile 0, 0 tiles grid 1 x 1)
+ (tile size 512 x 512)
+ (top left tile 0, 0 tiles grid 2 x 2)

Should we roll this change out? Change expected results?

[Darin Adler]

Skip the tests until we resolve the problem?

[Ryan Haddad]

This change is also causing ios-simulator testing to exit early due to crashes:
<https://build.webkit.org/results/Apple%20iOS%209%20Simulator%20Release%20WK2%20(Tests)/r197541%20(3629)/fast/frames/lots-of-iframes-crash-log.txt>

[Simon Fraser (smfr)]

Mac tests fixed via bug 155020.

[Simon Fraser (smfr)]

(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 272810 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=272810&action=review
> > 
> > > Source/WebCore/platform/graphics/EdgeSet.h:32
> > > +class EdgeSet {
> > 
> > The functionality of this class is a subset of BoxExtent in LengthBox.h. Why
> > do we add a new class? You can change the name of BoxExtent if you want.
> > Actually this is what I tried to do in
> > https://bugs.webkit.org/show_bug.cgi?id=147495 but it's never got reviewed.
> 
> BoxExtent should be changed to use this template.

I recant. Bug 155040.

[Simon Fraser (smfr)]

Reopening. The landed patch was missing significant functionality.

[Simon Fraser (smfr)]

Created attachment 273050 [details]
Patch

[Simon Fraser (smfr)]

https://trac.webkit.org/r197594