Bug 90342 - Make compositing updates incremental
Summary: Make compositing updates incremental
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
: 81239 (view as bug list)
Depends on:
Blocks: 158342 190245
  Show dependency treegraph
 
Reported: 2012-06-30 16:25 PDT by Simon Fraser (smfr)
Modified: 2019-02-13 09:30 PST (History)
11 users (show)

See Also:


Attachments
First cut, not ready for review. (32.31 KB, patch)
2012-06-30 16:28 PDT, Simon Fraser (smfr)
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch for EWS (160.60 KB, patch)
2018-10-15 15:20 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.65 MB, application/zip)
2018-10-15 16:23 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.22 MB, application/zip)
2018-10-15 16:42 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (3.14 MB, application/zip)
2018-10-15 17:14 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.88 MB, application/zip)
2018-10-15 17:43 PDT, EWS Watchlist
no flags Details
Patch (171.75 KB, patch)
2018-10-15 18:39 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.58 MB, application/zip)
2018-10-15 19:27 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.40 MB, application/zip)
2018-10-15 19:58 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.19 MB, application/zip)
2018-10-15 20:39 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (3.90 MB, application/zip)
2018-10-15 20:40 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.25 MB, application/zip)
2018-10-15 22:42 PDT, EWS Watchlist
no flags Details
Patch (174.68 KB, patch)
2018-10-15 22:53 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.37 MB, application/zip)
2018-10-15 23:58 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.96 MB, application/zip)
2018-10-16 00:10 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (3.79 MB, application/zip)
2018-10-16 00:52 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.06 MB, application/zip)
2018-10-16 00:53 PDT, EWS Watchlist
no flags Details
Patch (174.70 KB, patch)
2018-10-16 11:26 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.55 MB, application/zip)
2018-10-16 12:25 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (3.18 MB, application/zip)
2018-10-16 14:05 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (3.78 MB, application/zip)
2018-10-16 14:39 PDT, EWS Watchlist
no flags Details
Patch (177.84 KB, patch)
2018-10-16 18:10 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (181.83 KB, patch)
2018-10-17 09:24 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.40 MB, application/zip)
2018-10-17 10:26 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.94 MB, application/zip)
2018-10-17 10:37 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.15 MB, application/zip)
2018-10-17 11:23 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.94 MB, application/zip)
2018-10-17 17:32 PDT, EWS Watchlist
no flags Details
Patch (182.02 KB, patch)
2018-10-18 11:35 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.37 MB, application/zip)
2018-10-18 12:38 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.99 MB, application/zip)
2018-10-18 12:49 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.21 MB, application/zip)
2018-10-18 13:28 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.72 MB, application/zip)
2018-10-18 13:34 PDT, EWS Watchlist
no flags Details
Patch (181.72 KB, patch)
2018-10-21 05:28 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (181.77 KB, patch)
2018-10-21 06:01 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-sierra-wk2 (2.89 MB, application/zip)
2018-10-21 07:14 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.52 MB, application/zip)
2018-10-21 08:01 PDT, EWS Watchlist
no flags Details
Patch (189.20 KB, patch)
2018-11-01 00:20 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-sierra (699.42 KB, application/zip)
2018-11-01 01:34 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews105 for mac-sierra-wk2 (3.20 MB, application/zip)
2018-11-01 01:40 PDT, EWS Watchlist
no flags Details
Patch (187.61 KB, patch)
2018-11-07 07:06 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.62 MB, application/zip)
2018-11-07 10:22 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.53 MB, application/zip)
2018-11-07 10:27 PST, EWS Watchlist
no flags Details
Patch (200.49 KB, patch)
2018-11-09 14:59 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (200.16 KB, patch)
2018-11-09 15:44 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (200.16 KB, patch)
2018-11-09 15:52 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (199.95 KB, patch)
2018-11-09 16:17 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (4.28 MB, application/zip)
2018-11-09 17:43 PST, EWS Watchlist
no flags Details
Patch (200.54 KB, patch)
2018-11-10 08:05 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (200.51 KB, patch)
2018-11-10 14:03 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (208.34 KB, patch)
2018-11-10 18:47 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (216.63 KB, patch)
2018-11-11 11:19 PST, Simon Fraser (smfr)
koivisto: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.33 MB, application/zip)
2018-11-11 13:05 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2012-06-30 16:25:21 PDT
It's expensive to traverse the whole layer tree for compositing updates, and when only a portion of the tree has changed, this is wasted work. We should try to do incremental updates when possible.
Comment 1 Simon Fraser (smfr) 2012-06-30 16:28:01 PDT
Created attachment 150323 [details]
First cut, not ready for review.
Comment 2 WebKit Review Bot 2012-06-30 16:31:17 PDT Comment hidden (obsolete)
Comment 3 Early Warning System Bot 2012-06-30 16:33:22 PDT Comment hidden (obsolete)
Comment 4 WebKit Review Bot 2012-06-30 16:34:46 PDT Comment hidden (obsolete)
Comment 5 Early Warning System Bot 2012-06-30 16:35:16 PDT Comment hidden (obsolete)
Comment 6 Radar WebKit Bug Importer 2012-06-30 16:38:58 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2012-06-30 16:50:01 PDT Comment hidden (obsolete)
Comment 8 Simon Fraser (smfr) 2018-10-15 15:20:48 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-10-15 15:23:23 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-10-15 16:23:01 PDT Comment hidden (obsolete)
Comment 11 EWS Watchlist 2018-10-15 16:23:02 PDT Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-10-15 16:42:29 PDT Comment hidden (obsolete)
Comment 13 EWS Watchlist 2018-10-15 16:42:31 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2018-10-15 17:14:29 PDT Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-10-15 17:14:31 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-10-15 17:43:41 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2018-10-15 17:43:42 PDT Comment hidden (obsolete)
Comment 18 Simon Fraser (smfr) 2018-10-15 18:39:04 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2018-10-15 19:27:14 PDT Comment hidden (obsolete)
Comment 20 EWS Watchlist 2018-10-15 19:27:16 PDT Comment hidden (obsolete)
Comment 21 EWS Watchlist 2018-10-15 19:58:26 PDT Comment hidden (obsolete)
Comment 22 EWS Watchlist 2018-10-15 19:58:28 PDT Comment hidden (obsolete)
Comment 23 EWS Watchlist 2018-10-15 20:39:35 PDT Comment hidden (obsolete)
Comment 24 EWS Watchlist 2018-10-15 20:39:37 PDT Comment hidden (obsolete)
Comment 25 EWS Watchlist 2018-10-15 20:40:05 PDT Comment hidden (obsolete)
Comment 26 EWS Watchlist 2018-10-15 20:40:06 PDT Comment hidden (obsolete)
Comment 27 EWS Watchlist 2018-10-15 22:42:53 PDT Comment hidden (obsolete)
Comment 28 EWS Watchlist 2018-10-15 22:42:55 PDT Comment hidden (obsolete)
Comment 29 Simon Fraser (smfr) 2018-10-15 22:53:33 PDT Comment hidden (obsolete)
Comment 30 EWS Watchlist 2018-10-15 23:58:53 PDT Comment hidden (obsolete)
Comment 31 EWS Watchlist 2018-10-15 23:58:55 PDT Comment hidden (obsolete)
Comment 32 EWS Watchlist 2018-10-16 00:10:11 PDT Comment hidden (obsolete)
Comment 33 EWS Watchlist 2018-10-16 00:10:13 PDT Comment hidden (obsolete)
Comment 34 EWS Watchlist 2018-10-16 00:52:37 PDT Comment hidden (obsolete)
Comment 35 EWS Watchlist 2018-10-16 00:52:39 PDT Comment hidden (obsolete)
Comment 36 EWS Watchlist 2018-10-16 00:53:00 PDT Comment hidden (obsolete)
Comment 37 EWS Watchlist 2018-10-16 00:53:02 PDT Comment hidden (obsolete)
Comment 38 Simon Fraser (smfr) 2018-10-16 11:26:51 PDT Comment hidden (obsolete)
Comment 39 EWS Watchlist 2018-10-16 12:25:16 PDT Comment hidden (obsolete)
Comment 40 EWS Watchlist 2018-10-16 12:25:18 PDT Comment hidden (obsolete)
Comment 41 EWS Watchlist 2018-10-16 14:05:13 PDT Comment hidden (obsolete)
Comment 42 EWS Watchlist 2018-10-16 14:05:15 PDT Comment hidden (obsolete)
Comment 43 EWS Watchlist 2018-10-16 14:39:01 PDT Comment hidden (obsolete)
Comment 44 EWS Watchlist 2018-10-16 14:39:04 PDT Comment hidden (obsolete)
Comment 45 Simon Fraser (smfr) 2018-10-16 18:10:35 PDT
Created attachment 352531 [details]
Patch
Comment 46 Simon Fraser (smfr) 2018-10-17 09:24:07 PDT
Created attachment 352572 [details]
Patch
Comment 47 EWS Watchlist 2018-10-17 10:26:28 PDT Comment hidden (obsolete)
Comment 48 EWS Watchlist 2018-10-17 10:26:30 PDT Comment hidden (obsolete)
Comment 49 EWS Watchlist 2018-10-17 10:37:50 PDT Comment hidden (obsolete)
Comment 50 EWS Watchlist 2018-10-17 10:37:52 PDT Comment hidden (obsolete)
Comment 51 EWS Watchlist 2018-10-17 11:23:04 PDT Comment hidden (obsolete)
Comment 52 EWS Watchlist 2018-10-17 11:23:06 PDT Comment hidden (obsolete)
Comment 53 EWS Watchlist 2018-10-17 17:32:19 PDT Comment hidden (obsolete)
Comment 54 EWS Watchlist 2018-10-17 17:32:21 PDT Comment hidden (obsolete)
Comment 55 Simon Fraser (smfr) 2018-10-18 11:35:10 PDT
Created attachment 352711 [details]
Patch
Comment 56 EWS Watchlist 2018-10-18 12:38:13 PDT Comment hidden (obsolete)
Comment 57 EWS Watchlist 2018-10-18 12:38:16 PDT Comment hidden (obsolete)
Comment 58 EWS Watchlist 2018-10-18 12:49:19 PDT Comment hidden (obsolete)
Comment 59 EWS Watchlist 2018-10-18 12:49:21 PDT Comment hidden (obsolete)
Comment 60 EWS Watchlist 2018-10-18 13:28:37 PDT Comment hidden (obsolete)
Comment 61 EWS Watchlist 2018-10-18 13:28:40 PDT Comment hidden (obsolete)
Comment 62 EWS Watchlist 2018-10-18 13:34:07 PDT Comment hidden (obsolete)
Comment 63 EWS Watchlist 2018-10-18 13:34:09 PDT Comment hidden (obsolete)
Comment 64 Simon Fraser (smfr) 2018-10-21 05:28:52 PDT
Created attachment 352869 [details]
Patch
Comment 65 Simon Fraser (smfr) 2018-10-21 06:01:04 PDT
Created attachment 352870 [details]
Patch
Comment 66 EWS Watchlist 2018-10-21 07:14:40 PDT Comment hidden (obsolete)
Comment 67 EWS Watchlist 2018-10-21 07:14:43 PDT Comment hidden (obsolete)
Comment 68 EWS Watchlist 2018-10-21 08:01:16 PDT Comment hidden (obsolete)
Comment 69 EWS Watchlist 2018-10-21 08:01:18 PDT Comment hidden (obsolete)
Comment 70 Simon Fraser (smfr) 2018-11-01 00:20:49 PDT Comment hidden (obsolete)
Comment 71 EWS Watchlist 2018-11-01 00:29:46 PDT Comment hidden (obsolete)
Comment 72 EWS Watchlist 2018-11-01 01:34:15 PDT Comment hidden (obsolete)
Comment 73 EWS Watchlist 2018-11-01 01:34:17 PDT Comment hidden (obsolete)
Comment 74 EWS Watchlist 2018-11-01 01:40:24 PDT Comment hidden (obsolete)
Comment 75 EWS Watchlist 2018-11-01 01:40:26 PDT Comment hidden (obsolete)
Comment 76 Simon Fraser (smfr) 2018-11-07 07:06:27 PST
Created attachment 354091 [details]
Patch
Comment 77 EWS Watchlist 2018-11-07 10:22:32 PST Comment hidden (obsolete)
Comment 78 EWS Watchlist 2018-11-07 10:22:35 PST Comment hidden (obsolete)
Comment 79 EWS Watchlist 2018-11-07 10:27:45 PST Comment hidden (obsolete)
Comment 80 EWS Watchlist 2018-11-07 10:27:48 PST Comment hidden (obsolete)
Comment 81 Simon Fraser (smfr) 2018-11-09 14:59:24 PST
Created attachment 354390 [details]
Patch
Comment 82 Simon Fraser (smfr) 2018-11-09 15:44:26 PST
Created attachment 354396 [details]
Patch
Comment 83 Simon Fraser (smfr) 2018-11-09 15:52:59 PST
Created attachment 354400 [details]
Patch
Comment 84 Simon Fraser (smfr) 2018-11-09 16:17:35 PST
Created attachment 354406 [details]
Patch
Comment 85 EWS Watchlist 2018-11-09 17:43:44 PST
Comment on attachment 354406 [details]
Patch

Attachment 354406 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9931946

New failing tests:
compositing/video/video-clip-change-src.html
Comment 86 EWS Watchlist 2018-11-09 17:43:46 PST
Created attachment 354421 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 87 Simon Fraser (smfr) 2018-11-10 08:05:54 PST
Created attachment 354456 [details]
Patch
Comment 88 Simon Fraser (smfr) 2018-11-10 14:03:35 PST
Created attachment 354475 [details]
Patch
Comment 89 Simon Fraser (smfr) 2018-11-10 18:47:59 PST
Created attachment 354491 [details]
Patch
Comment 90 EWS Watchlist 2018-11-10 18:51:38 PST
Attachment 354491 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.h:397:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 91 Simon Fraser (smfr) 2018-11-11 11:19:05 PST
Created attachment 354508 [details]
Patch
Comment 92 EWS Watchlist 2018-11-11 11:22:00 PST
Attachment 354508 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.h:397:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 93 Simon Fraser (smfr) 2018-11-11 11:22:08 PST
*** Bug 81239 has been marked as a duplicate of this bug. ***
Comment 94 EWS Watchlist 2018-11-11 13:05:07 PST
Comment on attachment 354508 [details]
Patch

Attachment 354508 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9950641

New failing tests:
compositing/filters/sw-layer-overlaps-hw-shadow.html
Comment 95 EWS Watchlist 2018-11-11 13:05:10 PST
Created attachment 354510 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 96 Simon Fraser (smfr) 2018-11-11 15:43:36 PST
Failing test just needs iOS results.
Comment 97 Antti Koivisto 2018-11-12 06:58:26 PST
Comment on attachment 354508 [details]
Patch

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

r=me, nice!

> Source/WebCore/ChangeLog:33
> +        These changes fix a correctness issue where transform changes now trigger overlap re-evaluation, which causes
> +        more layer geometry updates than before. This regressed the MotionMark "Focus" test, when geometry updates
> +        triggered layer resizes as the filter blur radius changed, which then triggered repaints. This is fixed by
> +        excluding composited filters from the composited bounds (but still taking them into account for overlap).

Do we have some performance tests that progress?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2162
> +    if (madeLayer) {
>          updateBackdropFiltersRect();
> +        noteSublayersChanged(DontScheduleFlush);
> +    }

Why not schedule a flush?

> Source/WebCore/rendering/RenderLayer.cpp:279
>      , m_forcedStackingContext(rendererLayerModelObject.isMedia())
>      , m_zOrderListsDirty(false)
>      , m_normalFlowListDirty(true)
> +    , m_hadNegativeZOrderList(false)
>      , m_inResizeMode(false)
>      , m_scrollDimensionsDirty(true)
>      , m_hasSelfPaintingLayerDescendant(false)

Would be nice to turn these into normal bools (or OptionSets) at some point. Pretty sure memory optimization from bitfields is not meaningful.

> Source/WebCore/rendering/RenderLayer.cpp:617
> +    RenderLayer* layer = parent();

auto*

> Source/WebCore/rendering/RenderLayer.h:197
> +    enum CompositingStateFlag {

You might consider making this 'enum class CompositingState', 'CompositingFlag' or just 'Compositing' and dropping the then redundant word 'Compositing' from the value names.

> Source/WebCore/rendering/RenderLayer.h:256
> +    bool needsPostLayoutCompositingUpdate() const { return m_compositingDirtyBits.contains(NeedsPostLayoutCompositingUpdate); }

Not sure how all these accessors are better than making the enum public and exposing m_compositingDirtyBits via const accessor. They are not used that much.

> Source/WebCore/rendering/RenderLayer.h:261
> +    void setNeedsPostLayoutCompositingUpdate()
> +    {
> +        m_compositingDirtyBits.add(NeedsPostLayoutCompositingUpdate);
> +        setAncestorsHaveCompositingDirtyFlag(HasDescendantNeedingCompositingRequirementsTraversal);
> +    }

Do all these really need to be in the header? It is already big.

> Source/WebCore/rendering/RenderLayerBacking.cpp:629
>      } else
>          m_artificiallyInflatedBounds = false;
>  
> -    setCompositedBounds(layerBounds);
> +    return setCompositedBounds(layerBounds);

Could m_artificiallyInflatedBounds change even when the computed bounds stay the same? Should we return true in that case?

> Source/WebCore/rendering/RenderLayerBacking.cpp:661
> +// This can only update things that don't require up-to-date layout.
> +void RenderLayerBacking::updateConfigurationAfterStyleChange()

updateConfigurationAfterStyleChangeWithoutRequiringUpToDateLayout? :D

> Source/WebCore/rendering/RenderLayerCompositor.cpp:227
> +        , fullPaintOrderTraversalRequired(false)
> +        , descendantsRequireCompositingUpdate(false)
>          , ancestorHasTransformAnimation(false)

These could be switched to modern initialization.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:657
> +// Returns true on a successful update.
>  bool RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType updateType, RenderLayer* updateRoot)

enum return value would avoid the comment.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:661
> +    UNUSED_PARAM(updateType);
> +    UNUSED_PARAM(updateRoot);

Neither of these parameters is unused.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:691
> +        // Scrolling can affect overlap. FIXME: avoid for page scrolling.
> +        updateRoot->setDescendantsNeedCompositingRequirementsTraversal();

Sounds like a big FIXME!

> Source/WebCore/rendering/RenderLayerCompositor.cpp:697
> +    if (!updateRoot->hasDescendantNeedingCompositingRequirementsTraversal() && !m_compositing) {
> +        LOG_WITH_STREAM(Compositing, stream << " no compositing work to do");
>          return false;
> +    }

Why is this case considered 'unsuccessful'...

> Source/WebCore/rendering/RenderLayerCompositor.cpp:702
> +    if (!updateRoot->needsAnyCompositingTraversal()) {
> +        LOG_WITH_STREAM(Compositing, stream << " updateRoot has no dirty child and doesn't need update");
> +        return true;
> +    }

while this is considered 'successful'.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:802
> +    compositingState.fullPaintOrderTraversalRequired |= layer.needsCompositingRequirementsTraversal();
> +    compositingState.descendantsRequireCompositingUpdate |= layer.descendantsNeedCompositingRequirementsTraversal();

Using bit operators like this is slightly ugly.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1019
> +// We have to traverse unchanged layers to fill in the overlap map.
> +void RenderLayerCompositor::traverseUnchangedSubtree(RenderLayer* ancestorLayer, RenderLayer& layer, OverlapMap& overlapMap, CompositingState& compositingState, bool& descendantHas3DTransform)

Can results of this traversal be cached? Presumably the results don't change if the layers are unchanged.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1252
> +void RenderLayerCompositor::layerBecameNonComposited(const RenderLayer& layer)
> +{
> +    // Inform the inspector that the given RenderLayer was destroyed.
> +    InspectorInstrumentation::renderLayerDestroyed(&page(), layer);

Something here seems like a lie. Was the layer destroyed or did it just become non-composited?

> Source/WebCore/rendering/RenderLayerCompositor.h:142
> +    struct CompositingQueryData {

This seems to introduce concept of "compositing query" but there are no functions or anything else referencing this concept, so it is bit difficult to understand how it is used.

> Source/WebCore/rendering/RenderLayerCompositor.h:144
> +        RenderLayer::ViewportConstrainedNotCompositedReason nonCompositedForPositionReason { RenderLayer::NoNotCompositedReason };

NoNotCompositedReason :(

> Source/WebCore/rendering/RenderLayerCompositor.h:351
> +    bool needsToBeComposited(const RenderLayer&, CompositingQueryData&) const;
>      // Whether the layer has an intrinsic need for compositing layer.
> -    bool requiresCompositingLayer(const RenderLayer&, RenderLayer::ViewportConstrainedNotCompositedReason* = nullptr) const;
> +    bool requiresCompositingLayer(const RenderLayer&, CompositingQueryData&) const;

It is bit unclear if CompositingQueryData is an input, output or both for number of functions that take it. Could it be a (perhaps optional) return value when it is a pure output? Many clients initialize it in stack and invoke some function without doing anything between.

> Source/WebCore/rendering/RenderLayerCompositor.h:379
> +    enum class UpdateLevel {
> +        AllDescendants          = 1 << 0,
> +        CompositedChildren      = 1 << 1,
> +    };
>      // Recurses down the tree, parenting descendant compositing layers and collecting an array of child layers for the current compositing layer.
> -    void rebuildCompositingLayerTree(RenderLayer&, Vector<Ref<GraphicsLayer>>& childGraphicsLayersOfEnclosingLayer, int depth);
> +    void updateBackingAndHierarchy(RenderLayer&, Vector<Ref<GraphicsLayer>>& childGraphicsLayersOfEnclosingLayer, OptionSet<UpdateLevel> = { }, int depth = 0);

Why OptionSet? Doesn't 'AllDescendants' also cover 'CompositedChildren'? The logic might read better with a regular 3-value enum.
Comment 98 Antti Koivisto 2018-11-12 07:39:39 PST
Comment on attachment 354508 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:690
> +    if (hasNegativeZOrderList != m_hadNegativeZOrderList) {
> +        m_hadNegativeZOrderList = hasNegativeZOrderList;

The bit seems the reflect the current state so maybe m_has instead of m_had?

> Source/WebCore/rendering/RenderLayer.h:1180
> +    OptionSet<CompositingStateFlag> m_compositingDirtyBits;

Are these "state flags" or "dirty bits"? It would be nice if the type name and the field name matched.
Comment 99 Simon Fraser (smfr) 2018-11-12 08:47:29 PST
(In reply to Antti Koivisto from comment #97)
> Comment on attachment 354508 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354508&action=review
> 
> r=me, nice!
> 
> > Source/WebCore/ChangeLog:33
> > +        These changes fix a correctness issue where transform changes now trigger overlap re-evaluation, which causes
> > +        more layer geometry updates than before. This regressed the MotionMark "Focus" test, when geometry updates
> > +        triggered layer resizes as the filter blur radius changed, which then triggered repaints. This is fixed by
> > +        excluding composited filters from the composited bounds (but still taking them into account for overlap).
> 
> Do we have some performance tests that progress?

Yes, MotionMark shows it on the Focus test.

> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2162
> > +    if (madeLayer) {
> >          updateBackdropFiltersRect();
> > +        noteSublayersChanged(DontScheduleFlush);
> > +    }
> 
> Why not schedule a flush?

We're already inside a flush here.

> > Source/WebCore/rendering/RenderLayer.cpp:279
> >      , m_forcedStackingContext(rendererLayerModelObject.isMedia())
> >      , m_zOrderListsDirty(false)
> >      , m_normalFlowListDirty(true)
> > +    , m_hadNegativeZOrderList(false)
> >      , m_inResizeMode(false)
> >      , m_scrollDimensionsDirty(true)
> >      , m_hasSelfPaintingLayerDescendant(false)
> 
> Would be nice to turn these into normal bools (or OptionSets) at some point.
> Pretty sure memory optimization from bitfields is not meaningful.

Some pages can have thousands of RenderLayers, so I think some amount of space optimization is worthwhile.

C++17 (?) gives us initializers for bitfields.

> > Source/WebCore/rendering/RenderLayer.h:197
> > +    enum CompositingStateFlag {
> 
> You might consider making this 'enum class CompositingState',
> 'CompositingFlag' or just 'Compositing' and dropping the then redundant word
> 'Compositing' from the value names.
> 
> > Source/WebCore/rendering/RenderLayer.h:256
> > +    bool needsPostLayoutCompositingUpdate() const { return m_compositingDirtyBits.contains(NeedsPostLayoutCompositingUpdate); }
> 
> Not sure how all these accessors are better than making the enum public and
> exposing m_compositingDirtyBits via const accessor. They are not used that
> much.

I tried it the other way, but this way makes the code using the accessors more compact. Maybe grouping all the accessors together would make this header less noisy.

> > Source/WebCore/rendering/RenderLayer.h:261
> > +    void setNeedsPostLayoutCompositingUpdate()
> > +    {
> > +        m_compositingDirtyBits.add(NeedsPostLayoutCompositingUpdate);
> > +        setAncestorsHaveCompositingDirtyFlag(HasDescendantNeedingCompositingRequirementsTraversal);
> > +    }
> 
> Do all these really need to be in the header? It is already big.

I shrunk things down a bit.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:629
> >      } else
> >          m_artificiallyInflatedBounds = false;
> >  
> > -    setCompositedBounds(layerBounds);
> > +    return setCompositedBounds(layerBounds);
> 
> Could m_artificiallyInflatedBounds change even when the computed bounds stay
> the same? Should we return true in that case?

I think it's OK; all that matters whether the composited bounds changed (whether or not they were inflated).

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:227
> > +        , fullPaintOrderTraversalRequired(false)
> > +        , descendantsRequireCompositingUpdate(false)
> >          , ancestorHasTransformAnimation(false)
> 
> These could be switched to modern initialization.

Done

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:691
> > +        // Scrolling can affect overlap. FIXME: avoid for page scrolling.
> > +        updateRoot->setDescendantsNeedCompositingRequirementsTraversal();
> 
> Sounds like a big FIXME!

Yeah, filed bug 191546 to track.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:697
> > +    if (!updateRoot->hasDescendantNeedingCompositingRequirementsTraversal() && !m_compositing) {
> > +        LOG_WITH_STREAM(Compositing, stream << " no compositing work to do");
> >          return false;
> > +    }
> 
> Why is this case considered 'unsuccessful'...

Fixed.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:802
> > +    compositingState.fullPaintOrderTraversalRequired |= layer.needsCompositingRequirementsTraversal();
> > +    compositingState.descendantsRequireCompositingUpdate |= layer.descendantsNeedCompositingRequirementsTraversal();
> 
> Using bit operators like this is slightly ugly.

Is there a nicer way?

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1019
> > +// We have to traverse unchanged layers to fill in the overlap map.
> > +void RenderLayerCompositor::traverseUnchangedSubtree(RenderLayer* ancestorLayer, RenderLayer& layer, OverlapMap& overlapMap, CompositingState& compositingState, bool& descendantHas3DTransform)
> 
> Can results of this traversal be cached? Presumably the results don't change
> if the layers are unchanged.

We'd have to keep the overlap map around, and then know how to dirty it.


> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1252
> > +void RenderLayerCompositor::layerBecameNonComposited(const RenderLayer& layer)
> > +{
> > +    // Inform the inspector that the given RenderLayer was destroyed.
> > +    InspectorInstrumentation::renderLayerDestroyed(&page(), layer);
> 
> Something here seems like a lie. Was the layer destroyed or did it just
> become non-composited?

Agreed. Filed bug 191547.

> > Source/WebCore/rendering/RenderLayerCompositor.h:142
> > +    struct CompositingQueryData {
> 
> This seems to introduce concept of "compositing query" but there are no
> functions or anything else referencing this concept, so it is bit difficult
> to understand how it is used.

Yeah. The "query" is the "do you need to composite" question. This struct really just wraps up a bunch of in and out params to these functions. I'll rename this to RequiresCompositingData.

> > Source/WebCore/rendering/RenderLayerCompositor.h:144
> > +        RenderLayer::ViewportConstrainedNotCompositedReason nonCompositedForPositionReason { RenderLayer::NoNotCompositedReason };
> 
> NoNotCompositedReason :(

Not new :|

> > Source/WebCore/rendering/RenderLayerCompositor.h:351
> > +    bool needsToBeComposited(const RenderLayer&, CompositingQueryData&) const;
> >      // Whether the layer has an intrinsic need for compositing layer.
> > -    bool requiresCompositingLayer(const RenderLayer&, RenderLayer::ViewportConstrainedNotCompositedReason* = nullptr) const;
> > +    bool requiresCompositingLayer(const RenderLayer&, CompositingQueryData&) const;
> 
> It is bit unclear if CompositingQueryData is an input, output or both for
> number of functions that take it. Could it be a (perhaps optional) return
> value when it is a pure output? Many clients initialize it in stack and
> invoke some function without doing anything between.

It has both inputs and outputs.

> > Source/WebCore/rendering/RenderLayerCompositor.h:379
> > +    enum class UpdateLevel {
> > +        AllDescendants          = 1 << 0,
> > +        CompositedChildren      = 1 << 1,
> > +    };
> >      // Recurses down the tree, parenting descendant compositing layers and collecting an array of child layers for the current compositing layer.
> > -    void rebuildCompositingLayerTree(RenderLayer&, Vector<Ref<GraphicsLayer>>& childGraphicsLayersOfEnclosingLayer, int depth);
> > +    void updateBackingAndHierarchy(RenderLayer&, Vector<Ref<GraphicsLayer>>& childGraphicsLayersOfEnclosingLayer, OptionSet<UpdateLevel> = { }, int depth = 0);
> 
> Why OptionSet? Doesn't 'AllDescendants' also cover 'CompositedChildren'? The
> logic might read better with a regular 3-value enum.

AllDescendants trumps CompositedChildren, yes. With a 3-state, there would be more if-then code when toggling CompositedChildren.
Comment 100 Simon Fraser (smfr) 2018-11-12 08:50:30 PST
(In reply to Antti Koivisto from comment #98)
> Comment on attachment 354508 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354508&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:690
> > +    if (hasNegativeZOrderList != m_hadNegativeZOrderList) {
> > +        m_hadNegativeZOrderList = hasNegativeZOrderList;
> 
> The bit seems the reflect the current state so maybe m_has instead of m_had?

It's "had" in the sense that last time we updated z-order lists, we saw negative z children, but after lists are dirtied (which clears them), we are in a limbo state.

> > Source/WebCore/rendering/RenderLayer.h:1180
> > +    OptionSet<CompositingStateFlag> m_compositingDirtyBits;
> 
> Are these "state flags" or "dirty bits"? It would be nice if the type name
> and the field name matched.

It's OptionSet<Compositing> m_compositingDirtyBits now. "state" implies information about the steady state of the layer; these are dirty bits.
Comment 101 Simon Fraser (smfr) 2018-11-12 09:14:46 PST
https://trac.webkit.org/r238090
Comment 102 Antti Koivisto 2018-11-12 11:30:34 PST
> > Using bit operators like this is slightly ugly.
> 
> Is there a nicer way?

Both if() and operator|| avoid unnecessary evaluation of the right side (though it doesn't matter in practice here).
Comment 103 Charlie Turner 2018-11-13 02:12:22 PST
This breaks release builds with logging enabled. I've noticed a few times now that there's some confusion about when to use if !LOG_DISABLED, ifndef NDEBUG and now in this patch if ENABLE(TREE_DEBUGGING). We need a bot building release with logging enabled to catch these imo, I know I'm not the only one building like this, because full debug builds are too slow on my hardware :). Could someone have a look at 191583 where I've applied some build fixes for release+logging
Comment 104 Jon Lee 2019-02-13 09:30:42 PST
rdar://problem/11784760