Bug 240906

Summary:

Make shouldApplyContainment methods more flexible

Product:

WebKit

Reporter:

Rob Buis <rbuis>

Component:

CSS

Assignee:

Rob Buis <rbuis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

changseok, esprehn+autocc, ews-watchlist, glenn, jfernandez, kondapallykalyan, pdr, rego, simon.fraser, svillar, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari 15

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

243534

Attachments:

Description	Flags
Patch	none
Patch	none

Description Rob Buis 2022-05-25 06:42:54 PDT

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().

Comment 1 Rob Buis 2022-05-25 07:18:26 PDT

Created attachment 459757 [details]
Patch

Comment 2 Simon Fraser (smfr) 2022-05-25 13:31:07 PDT

Comment on attachment 459757 [details]
Patch

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

Comment 3 Rob Buis 2022-05-25 14:00:55 PDT

(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.

Comment 4 Rob Buis 2022-05-26 02:18:17 PDT

Created attachment 459779 [details]
Patch

Comment 5 Rob Buis 2022-05-26 08:09:13 PDT

(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.

Comment 6 Radar WebKit Bug Importer 2022-06-01 06:43:13 PDT

<rdar://problem/94218126>

Comment 7 Rob Buis 2022-06-08 05:15:20 PDT

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

Comment 8 Rob Buis 2022-06-08 05:18:26 PDT

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

Comment 9 Rob Buis 2022-07-29 11:28:03 PDT

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

Comment 10 Rob Buis 2022-08-10 01:25:55 PDT

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

Comment 11 EWS 2022-08-11 00:40:00 PDT

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

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