Bug 201403 - Cache "compositingAncestor" during the compositing updateBackingAndHierarchy() tree walk
Summary: Cache "compositingAncestor" during the compositing updateBackingAndHierarchy(...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-02 10:03 PDT by Simon Fraser (smfr)
Modified: 2019-09-03 12:56 PDT (History)
5 users (show)

See Also:


Attachments
Patch (26.76 KB, patch)
2019-09-02 10:04 PDT, Simon Fraser (smfr)
koivisto: 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) 2019-09-02 10:03:20 PDT
Cache "compositingAncestor" during the compositing updateBackingAndHierarchy() tree walk
Comment 1 Simon Fraser (smfr) 2019-09-02 10:04:59 PDT
Created attachment 377855 [details]
Patch
Comment 2 Daniel Bates 2019-09-02 12:11:40 PDT
Comment on attachment 377855 [details]
Patch

This patch looks good. In my opinion it would benefit by using more references and less pointers because:
1. I do not see null checks so the code looks to be written assuming non-nullness
2. Because of (1) a newcomer to this code may think RenderLayer can be null and add null checks or asserts when they are not necessary
Comment 3 Antti Koivisto 2019-09-03 10:32:32 PDT
Comment on attachment 377855 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.h:519
> -    OptionSet<ScrollCoordinationRole> coordinatedScrollingRolesForLayer(const RenderLayer&) const;
> +    OptionSet<ScrollCoordinationRole> coordinatedScrollingRolesForLayer(const RenderLayer&, const RenderLayer* compositingAncestor) const;
>  
>      // Returns the ScrollingNodeID which acts as the parent for children.
> -    ScrollingNodeID updateScrollCoordinationForLayer(RenderLayer&, struct ScrollingTreeState&, OptionSet<ScrollingNodeChangeFlags>);
> +    ScrollingNodeID updateScrollCoordinationForLayer(RenderLayer&, const RenderLayer* compositingAncestor, struct ScrollingTreeState&, OptionSet<ScrollingNodeChangeFlags>);

It might be better to pass in UpdateBackingTraversalState& instead of RenderLayer* to these functions. It makes clear in which context these functions are used in and potentially allows caching more things in future.
Comment 4 Simon Fraser (smfr) 2019-09-03 12:51:07 PDT
(In reply to Daniel Bates from comment #2)
> Comment on attachment 377855 [details]
> Patch
> 
> This patch looks good. In my opinion it would benefit by using more
> references and less pointers because:
> 1. I do not see null checks so the code looks to be written assuming
> non-nullness

compositedAncestor can be null (e.g. for the root layer).
Comment 5 Simon Fraser (smfr) 2019-09-03 12:53:29 PDT
(In reply to Antti Koivisto from comment #3)
> Comment on attachment 377855 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377855&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.h:519
> > -    OptionSet<ScrollCoordinationRole> coordinatedScrollingRolesForLayer(const RenderLayer&) const;
> > +    OptionSet<ScrollCoordinationRole> coordinatedScrollingRolesForLayer(const RenderLayer&, const RenderLayer* compositingAncestor) const;
> >  
> >      // Returns the ScrollingNodeID which acts as the parent for children.
> > -    ScrollingNodeID updateScrollCoordinationForLayer(RenderLayer&, struct ScrollingTreeState&, OptionSet<ScrollingNodeChangeFlags>);
> > +    ScrollingNodeID updateScrollCoordinationForLayer(RenderLayer&, const RenderLayer* compositingAncestor, struct ScrollingTreeState&, OptionSet<ScrollingNodeChangeFlags>);
> 
> It might be better to pass in UpdateBackingTraversalState& instead of
> RenderLayer* to these functions. It makes clear in which context these
> functions are used in and potentially allows caching more things in future.

For now, I don't want UpdateBackingTraversalState to leak out of the tree walking code, but if it gains most stuff for caching, I can pass it around later.
Comment 6 Simon Fraser (smfr) 2019-09-03 12:55:36 PDT
https://trac.webkit.org/changeset/249440/webkit
Comment 7 Radar WebKit Bug Importer 2019-09-03 12:56:19 PDT
<rdar://problem/54987053>