Bug 227948

Summary:

[Performance test][css-contain] Add test to contain: size layout

Product:

WebKit

Reporter:

cathiechen <cathiechen>

Component:

CSS

Assignee:

cathiechen <cathiechen>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, ews-watchlist, glenn, jbedard, jfernandez, rbuis, rego, rniwa, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

172026

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

cathiechen

Reported 2021-07-14 06:44:07 PDT

To test the scenario to change the content of size and layout containment.

Attachments
Patch (5.85 KB, patch) 2021-07-15 20:23 PDT, cathiechen	no flags	Details Formatted Diff Diff
Patch (4.22 KB, patch) 2021-07-15 20:33 PDT, cathiechen	no flags	Details Formatted Diff Diff
Patch (3.53 KB, patch) 2021-07-16 09:34 PDT, cathiechen	no flags	Details Formatted Diff Diff
Patch (3.53 KB, patch) 2021-07-23 20:06 PDT, cathiechen	no flags	Details Formatted Diff Diff
Patch (3.56 KB, patch) 2021-07-24 19:50 PDT, cathiechen	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

cathiechen

Comment 1 2021-07-15 20:23:45 PDT

Created attachment 433649 [details] Patch

cathiechen

Comment 2 2021-07-15 20:33:03 PDT

Created attachment 433650 [details] Patch

Ryosuke Niwa

Comment 3 2021-07-16 00:53:06 PDT

Comment on attachment 433650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433650&action=review > PerformanceTests/CSS/css-contain/css-contain-change-text.html:1 > +<!DOCTYPE html> This should be under PerfomanceTests/Layout. > PerformanceTests/CSS/css-contain/css-contain-change-text.html:70 > + PerfTestRunner.measureTime({ Please use measureRunsPerSecond instead. > PerformanceTests/CSS/css-contain/css-contain-change-text.html:75 > + PerfTestRunner.forceLayout(); I don't think we need a helper function like this. Just call getBoundingClientRect() on some node manually instead. > PerformanceTests/resources/runner.js:62 > + doc = doc || document; > + if (doc.body) > + doc.body.offsetHeight; > + else if (doc.documentElement) > + doc.documentElement.offsetHeight; Are you copying this code from somewhere else? If so, you need to give the credit.

Manuel Rego Casasnovas

Comment 4 2021-07-16 03:29:10 PDT

Do the performance bots in WebKit run with experimental flags enabled? If not I'm not sure if it's really useful to land perf tests for CSS Containment yet, as the bots are not going to notice any optimization based on that. WDYT?

cathiechen

Comment 5 2021-07-16 03:53:12 PDT

Comment on attachment 433650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433650&action=review Hi Ryosuke, Thanks for the review! >> PerformanceTests/CSS/css-contain/css-contain-change-text.html:1 >> +<!DOCTYPE html> > > This should be under PerfomanceTests/Layout. Done. >> PerformanceTests/CSS/css-contain/css-contain-change-text.html:70 >> + PerfTestRunner.measureTime({ > > Please use measureRunsPerSecond instead. Done! >> PerformanceTests/CSS/css-contain/css-contain-change-text.html:75 >> + PerfTestRunner.forceLayout(); > > I don't think we need a helper function like this. Just call getBoundingClientRect() on some node manually instead. Done! >> PerformanceTests/resources/runner.js:62 >> + doc.documentElement.offsetHeight; > > Are you copying this code from somewhere else? If so, you need to give the credit. Yeah, I copy this and the test from Chromium. I'll explain it in changelog and cc the reviewers.

cathiechen

Comment 6 2021-07-16 09:34:40 PDT

Created attachment 433680 [details] Patch

cathiechen

Comment 7 2021-07-16 09:44:02 PDT

(In reply to Manuel Rego Casasnovas from comment #4) > Do the performance bots in WebKit run with experimental flags enabled? > > If not I'm not sure if it's really useful to land perf tests for CSS > Containment yet, as the bots are not going to notice any optimization based > on that. > > WDYT? Nope, the experimental flags are not enabled while I run it locally. Maybe there is a way to enable the experimental flag?

Radar WebKit Bug Importer

Comment 8 2021-07-21 06:45:16 PDT

<rdar://problem/80894989>

cathiechen

Comment 9 2021-07-23 02:33:41 PDT

The test result I ran locally with containment enabled and disable are: 14.87580277047857 runs/s to 6.369440777576201 runs/s Analyzed the profiler data. The performance improvement is related to the changes to objectIsRelayoutBoundary(), which makes layout containment as a relayout boundary. Then in FrameViewLayoutContext::layout, the layout could start from m_subtreeLayoutRoot instead of RenderView. Then in FrameView::didLayout, it reduces a lot of RenderLayer::updateLayerPositions.

Ryosuke Niwa

Comment 10 2021-07-23 13:58:23 PDT

Comment on attachment 433680 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433680&action=review > PerformanceTests/Layout/css-contain-change-size.html:28 > + let flexibleNodes = []; > + let NUM_ROWS = 10; > + let NUM_COLS = 10; These should all be const. Also, please call them rowCount & colCount. No need to capitalize them like this. > PerformanceTests/Layout/css-contain-change-size.html:31 > + let container = document.createElement('div'); const. > PerformanceTests/Layout/css-contain-change-size.html:36 > + let row = document.createElement('div'); const. > PerformanceTests/Layout/css-contain-change-size.html:40 > + let cell = document.createElement('div'); const. > PerformanceTests/Layout/css-contain-change-size.html:42 > + let flexibleNode = document.createElement('div'); const. > PerformanceTests/Layout/css-contain-change-size.html:43 > + flexibleNode.style.width = (100 * Math.random()).toFixed(0) + "px"; Let's use PerfTestRunner.random to make it clear this won't be truly random. > PerformanceTests/Layout/css-contain-change-size.html:66 > + for (let i = 0; i < flexibleNodes.length; i++) { Use for-of loop like this: for (const node of flexibleNodes) { > PerformanceTests/Layout/css-contain-change-size.html:67 > + flexibleNodes[i].style.width = (100 * Math.random()).toFixed(0) + 'px'; Ditto about using PerfTestRunner.random instead of Math.random.

cathiechen

Comment 11 2021-07-23 19:45:03 PDT

Comment on attachment 433680 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433680&action=review >> PerformanceTests/Layout/css-contain-change-size.html:28 >> + let NUM_COLS = 10; > > These should all be const. Also, please call them rowCount & colCount. > No need to capitalize them like this. I see... Done, thanks! >> PerformanceTests/Layout/css-contain-change-size.html:31 >> + let container = document.createElement('div'); > > const. Done. >> PerformanceTests/Layout/css-contain-change-size.html:36 >> + let row = document.createElement('div'); > > const. Done. >> PerformanceTests/Layout/css-contain-change-size.html:40 >> + let cell = document.createElement('div'); > > const. Done. >> PerformanceTests/Layout/css-contain-change-size.html:42 >> + let flexibleNode = document.createElement('div'); > > const. Done. >> PerformanceTests/Layout/css-contain-change-size.html:43 >> + flexibleNode.style.width = (100 * Math.random()).toFixed(0) + "px"; > > Let's use PerfTestRunner.random to make it clear this won't be truly random. Done! >> PerformanceTests/Layout/css-contain-change-size.html:66 >> + for (let i = 0; i < flexibleNodes.length; i++) { > > Use for-of loop like this: for (const node of flexibleNodes) { Done, thanks! >> PerformanceTests/Layout/css-contain-change-size.html:67 >> + flexibleNodes[i].style.width = (100 * Math.random()).toFixed(0) + 'px'; > > Ditto about using PerfTestRunner.random instead of Math.random. Done!

cathiechen

Comment 12 2021-07-23 20:06:56 PDT

Created attachment 434157 [details] Patch

Ryosuke Niwa

Comment 13 2021-07-24 00:50:28 PDT

Comment on attachment 434157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434157&action=review > PerformanceTests/Layout/css-contain-change-size.html:26 > + let flexibleNodes = []; Since we're not assigning a new value to this variable, this should also be const. JavaScript const semantics ins't like that of C++. It doesn't mean that the object referenced by the variable isn't mutable. So even if we declared this const variable, we can still push more contents to the array. Also, we should call this widthChangingElements or resizingElements. "flexibleNodes" seems rather vague. > PerformanceTests/Layout/css-contain-change-size.html:38 > + row.style.top = top + "px"; We should probably use single quotation marks throughout the script for consistency. > PerformanceTests/Layout/css-contain-change-size.html:44 > + flexibleNode.style.width = (100 * PerfTestRunner.random()).toFixed(0) + "px"; > + flexibleNode.style.height = "100px"; Ditto. > PerformanceTests/Layout/css-contain-change-size.html:46 > + cell.style.left = left + "px"; Ditto. > PerformanceTests/Layout/css-contain-change-size.html:63 > + description: "Measures performance of changing widths of nodes.", Ditto.

cathiechen

Comment 14 2021-07-24 19:47:13 PDT

Comment on attachment 434157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434157&action=review >> PerformanceTests/Layout/css-contain-change-size.html:26 >> + let flexibleNodes = []; > > Since we're not assigning a new value to this variable, this should also be const. > JavaScript const semantics ins't like that of C++. > It doesn't mean that the object referenced by the variable isn't mutable. > So even if we declared this const variable, we can still push more contents to the array. > Also, we should call this widthChangingElements or resizingElements. "flexibleNodes" seems rather vague. Done! Thanks for the explanation! >> PerformanceTests/Layout/css-contain-change-size.html:38 >> + row.style.top = top + "px"; > > We should probably use single quotation marks throughout the script for consistency. Done, thanks! >> PerformanceTests/Layout/css-contain-change-size.html:44 >> + flexibleNode.style.height = "100px"; > > Ditto. Done. >> PerformanceTests/Layout/css-contain-change-size.html:46 >> + cell.style.left = left + "px"; > > Ditto. Done. >> PerformanceTests/Layout/css-contain-change-size.html:63 >> + description: "Measures performance of changing widths of nodes.", > > Ditto. Done.

cathiechen

Comment 15 2021-07-24 19:50:07 PDT

Created attachment 434177 [details] Patch

EWS

Comment 16 2021-07-25 06:01:05 PDT

Committed r280286 (239944@main): <https://commits.webkit.org/239944@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434177 [details].

Note You need to log in before you can comment on or make changes to this bug.