| 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: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Local Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 172026 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
cathiechen
2021-07-14 06:44:07 PDT
Created attachment 433649 [details]
Patch
Created attachment 433650 [details]
Patch
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. 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? 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. Created attachment 433680 [details]
Patch
(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? 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. 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. 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! Created attachment 434157 [details]
Patch
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. 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. Created attachment 434177 [details]
Patch
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]. |