Bug 240906 – Make shouldApplyContainment methods more flexible

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rob Buis]

In r293943 some shouldApplyContainment methods were optimised, however
they are not flexible enough to deal with the content-visibility feature,
which in certain cases can influence containment. So to be flexible use
lambdas that return a bool instead of relying on effectiveContainment().

[Rob Buis]

Created attachment 459757 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 459757 [details]
Patch

These functions are hot because they fall into the "containingBlock" code path. Does this affect performance?

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 459757 [details]
> Patch
> 
> These functions are hot because they fall into the "containingBlock" code
> path. Does this affect performance?

Are you thinking of canContainFixedPositionObjects/canContainAbsolutelyPositionedObjects or more?
If the lambda's are nicely inlined the performance should be the same, but I never checked efficiency for that (I guess I could try godbold.org tomorrow). An older patch used bool parameters for shouldApplyLayoutOrPaintContainment/shouldApplySizeOrStyleContainment, to me it does not look as nice as lambdas, but it pretty much guarantees same perf as we have now, and it works to support content-visibility later on, so I do not mind using that instead.

[Rob Buis]

Created attachment 459779 [details]
Patch

[Rob Buis]

(In reply to Rob Buis from comment #3)
> (In reply to Simon Fraser (smfr) from comment #2)
> > Comment on attachment 459757 [details]
> > Patch
> > 
> > These functions are hot because they fall into the "containingBlock" code
> > path. Does this affect performance?
> 
> Are you thinking of
> canContainFixedPositionObjects/canContainAbsolutelyPositionedObjects or more?
> If the lambda's are nicely inlined the performance should be the same, but I
> never checked efficiency for that (I guess I could try godbold.org
> tomorrow). An older patch used bool parameters for
> shouldApplyLayoutOrPaintContainment/shouldApplySizeOrStyleContainment, to me
> it does not look as nice as lambdas, but it pretty much guarantees same perf
> as we have now, and it works to support content-visibility later on, so I do
> not mind using that instead.

From checking godbolt.org output the lambdas do seem to generate quite some extra instructions, so I changed the patch to use boolean parameters instead.

[Radar WebKit Bug Importer]

<rdar://problem/94218126>

[Rob Buis]

Pull request: https://github.com/WebKit/WebKit/pull/1373

[Rob Buis]

Pull request: https://github.com/WebKit/WebKit/pull/1374

[Rob Buis]

Pull request: https://github.com/WebKit/WebKit/pull/2857

[Rob Buis]

Pull request: https://github.com/WebKit/WebKit/pull/3182

[EWS]

Committed 253324@main (6c79952a8e99): <https://commits.webkit.org/253324@main>

Reviewed commits have been landed. Closing PR #3182 and removing active labels.