Bug 154985 - Use larger tiles when possible to reduce per-tile painting overhead
Summary: Use larger tiles when possible to reduce per-tile painting overhead
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-03 15:26 PST by Simon Fraser (smfr)
Modified: 2016-03-04 15:33 PST (History)
6 users (show)

See Also:


Attachments
Patch (61.75 KB, patch)
2016-03-03 15:36 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (799.97 KB, application/zip)
2016-03-03 16:24 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.02 MB, application/zip)
2016-03-03 16:27 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (922.84 KB, application/zip)
2016-03-03 16:35 PST, Build Bot
no flags Details
Patch (69.43 KB, patch)
2016-03-03 17:44 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (22.98 KB, patch)
2016-03-04 15:08 PST, Simon Fraser (smfr)
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2016-03-03 15:26:54 PST
Use larger tiles when possible to reduce per-tile painting overhead
Comment 1 Simon Fraser (smfr) 2016-03-03 15:36:40 PST
Created attachment 272782 [details]
Patch
Comment 2 Tim Horton 2016-03-03 15:46:23 PST
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?
Comment 3 Build Bot 2016-03-03 16:24:04 PST
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
Comment 4 Build Bot 2016-03-03 16:24:07 PST
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
Comment 5 Build Bot 2016-03-03 16:27:37 PST
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
Comment 6 Build Bot 2016-03-03 16:27:40 PST
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
Comment 7 Build Bot 2016-03-03 16:35:17 PST
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
Comment 8 Build Bot 2016-03-03 16:35:20 PST
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
Comment 9 Simon Fraser (smfr) 2016-03-03 17:44:12 PST
Created attachment 272810 [details]
Patch
Comment 10 Simon Fraser (smfr) 2016-03-03 18:27:07 PST
https://trac.webkit.org/r197541
Comment 11 Simon Fraser (smfr) 2016-03-03 18:27:15 PST
rdar://problem/23635219
Comment 12 Said Abou-Hallawa 2016-03-03 18:28:24 PST
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.
Comment 13 Simon Fraser (smfr) 2016-03-03 22:38:19 PST
(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.
Comment 14 Antti Koivisto 2016-03-04 04:59:32 PST
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;
}
Comment 15 Said Abou-Hallawa 2016-03-04 08:32:39 PST
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.
Comment 16 Darin Adler 2016-03-04 08:37:06 PST
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?
Comment 17 Darin Adler 2016-03-04 08:38:55 PST
Skip the tests until we resolve the problem?
Comment 18 Ryan Haddad 2016-03-04 09:59:37 PST
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>
Comment 19 Simon Fraser (smfr) 2016-03-04 10:49:30 PST
Mac tests fixed via bug 155020.
Comment 20 Simon Fraser (smfr) 2016-03-04 13:44:44 PST
(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.
Comment 21 Simon Fraser (smfr) 2016-03-04 15:04:39 PST
Reopening. The landed patch was missing significant functionality.
Comment 22 Simon Fraser (smfr) 2016-03-04 15:08:14 PST
Created attachment 273050 [details]
Patch
Comment 23 Simon Fraser (smfr) 2016-03-04 15:33:04 PST
https://trac.webkit.org/r197594