Issue 1405953005: Avoid multiple recursions through the tree when calculating percent height

Issue 1405953005: Avoid multiple recursions through the tree when calculating percent height (Closed)

Created:
5 years, 1 month ago by rhogan

Modified:
5 years, 1 month ago

Reviewers:
leviw_travelin_and_unemployed

CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1

Base URL:
https://chromium.googlesource.com/chromium/src.git@322039-2

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Avoid multiple recursions through the tree when calculating percent height Test=PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html BUG=512545 Committed: https://crrev.com/74e95b29247f21620cb94fdf72e7e1051fff80c2 Cr-Commit-Position: refs/heads/master@{#358640}

Patch Set 1 #

Patch Set 2 : Updated #

Patch Set 3 : Updated #

Total comments: 2

Patch Set 4 : Updated #

Created: 5 years, 1 month ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+19 lines, -4 lines)			Patch
	M	third_party/WebKit/Source/core/layout/LayoutBox.cpp	View	1 2 3	1 chunk	+19 lines, -4 lines	0 comments	Download

Messages

Total messages: 16 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

leviw_travelin_and_unemployed

https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html File third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html (right): https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html#newcode1 third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html:1: <!DOCTYPE html> What would you say to landing this ...

5 years, 1 month ago (2015-10-29 21:44:10 UTC) #4

leviw_travelin_and_unemployed

https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html File third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html (right): https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html#newcode6 third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html:6: .container { Also, it seems like this could just ...

5 years, 1 month ago (2015-10-29 21:44:58 UTC) #5

rhogan

On 2015/10/29 at 21:44:58, leviw wrote: > https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html > File third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html (right): > > https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html#newcode6 ...

5 years, 1 month ago (2015-11-03 19:43:25 UTC) #7

leviw_travelin_and_unemployed

On 2015/11/03 at 19:43:25, robhogan wrote: > On 2015/10/29 at 21:44:58, leviw wrote: > > ...

5 years, 1 month ago (2015-11-03 21:14:12 UTC) #8

leviw_travelin_and_unemployed

On 2015/11/03 at 21:14:12, leviw wrote: > On 2015/11/03 at 19:43:25, robhogan wrote: > > ...

5 years, 1 month ago (2015-11-03 21:17:47 UTC) #9

leviw_travelin_and_unemployed

On 2015/11/03 at 21:17:47, leviw wrote: > On 2015/11/03 at 21:14:12, leviw wrote: > > ...

5 years, 1 month ago (2015-11-03 21:26:30 UTC) #10

On 2015/11/03 at 21:17:47, leviw wrote:
> On 2015/11/03 at 21:14:12, leviw wrote:
> > On 2015/11/03 at 19:43:25, robhogan wrote:
> > > On 2015/10/29 at 21:44:58, leviw wrote:
> > > >
https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/Perf...
> > > > File
third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html
(right):
> > > > 
> > > >
https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/Perf...
> > > >
third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html:6:
.container {
> > > > Also, it seems like this could just be "div" :)
> > > 
> > > That has gathered some data now. Oddly enough, its poor performance only
shows up on the Android and Win bots. It doesn't behave poorly on the Linux or
Mac bots. No idea what's going on there. It runs at less than 1 run per second
for me locally.
> > > 
> > > OK to land?
> > 
> > I think we need to figure out your test case, first...
> > 
> > Here are my local results on today's canary:
> > values 2823.96234716872, 3167.9788801408263, 3003.9799734668595,
2873.295022732998, 2793.2960893854674, 3109.312604582613, 3254.644969033567,
2543.9660804522723, 262.6631644911388, 1.3286211569633022, 1837.33333333332,
1.3326847600834233, 1061.3262578249746, 1281.324791168054, 1069.319075745672,
227.99848001013186, 263.99824001173164, 218.6652088986081, 661.3156982480496,
817.3224357008531 runs/s
> > avg 1563.7181979156453 runs/s
> > median 1175.321933456863 runs/s
> > stdev 1247.151978828884 runs/s
> > min 1.3286211569633022 runs/s
> > max 3254.644969033567 runs/s
> > 
> > As low as 1.3, as high as 3,254!
> 
> Man.... reminding myself of the test... that makes no damn sense!

Making the test
        function test() {
            style.display = "block";
            PerfTestRunner.forceLayoutOrFullFrame();
            style.display = "none";
            PerfTestRunner.forceLayoutOrFullFrame();
        }

fixes it

rhogan

On 2015/11/03 at 21:26:30, leviw wrote: > On 2015/11/03 at 21:17:47, leviw wrote: > > ...

5 years, 1 month ago (2015-11-09 08:34:35 UTC) #11

On 2015/11/03 at 21:26:30, leviw wrote:
> On 2015/11/03 at 21:17:47, leviw wrote:
> > On 2015/11/03 at 21:14:12, leviw wrote:
> > > On 2015/11/03 at 19:43:25, robhogan wrote:
> > > > On 2015/10/29 at 21:44:58, leviw wrote:
> > > > >
https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/Perf...
> > > > > File
third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html
(right):
> > > > > 
> > > > >
https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/Perf...
> > > > >
third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html:6:
.container {
> > > > > Also, it seems like this could just be "div" :)
> > > > 
> > > > That has gathered some data now. Oddly enough, its poor performance only
shows up on the Android and Win bots. It doesn't behave poorly on the Linux or
Mac bots. No idea what's going on there. It runs at less than 1 run per second
for me locally.
> > > > 
> > > > OK to land?
> > > 
> > > I think we need to figure out your test case, first...
> > > 
> > > Here are my local results on today's canary:
> > > values 2823.96234716872, 3167.9788801408263, 3003.9799734668595,
2873.295022732998, 2793.2960893854674, 3109.312604582613, 3254.644969033567,
2543.9660804522723, 262.6631644911388, 1.3286211569633022, 1837.33333333332,
1.3326847600834233, 1061.3262578249746, 1281.324791168054, 1069.319075745672,
227.99848001013186, 263.99824001173164, 218.6652088986081, 661.3156982480496,
817.3224357008531 runs/s
> > > avg 1563.7181979156453 runs/s
> > > median 1175.321933456863 runs/s
> > > stdev 1247.151978828884 runs/s
> > > min 1.3286211569633022 runs/s
> > > max 3254.644969033567 runs/s
> > > 
> > > As low as 1.3, as high as 3,254!
> > 
> > Man.... reminding myself of the test... that makes no damn sense!
> 
> Making the test
>         function test() {
>             style.display = "block";
>             PerfTestRunner.forceLayoutOrFullFrame();
>             style.display = "none";
>             PerfTestRunner.forceLayoutOrFullFrame();
>         }
> 
> fixes it

OK to land this now?

leviw_travelin_and_unemployed

On 2015/11/09 at 08:34:35, robhogan wrote: > On 2015/11/03 at 21:26:30, leviw wrote: > > ...

5 years, 1 month ago (2015-11-09 19:11:16 UTC) #12

On 2015/11/09 at 08:34:35, robhogan wrote:
> On 2015/11/03 at 21:26:30, leviw wrote:
> > On 2015/11/03 at 21:17:47, leviw wrote:
> > > On 2015/11/03 at 21:14:12, leviw wrote:
> > > > On 2015/11/03 at 19:43:25, robhogan wrote:
> > > > > On 2015/10/29 at 21:44:58, leviw wrote:
> > > > > >
https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/Perf...
> > > > > > File
third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html
(right):
> > > > > > 
> > > > > >
https://codereview.chromium.org/1405953005/diff/40001/third_party/WebKit/Perf...
> > > > > >
third_party/WebKit/PerformanceTests/Layout/nested-blocks-with-percent-height-and-max-height.html:6:
.container {
> > > > > > Also, it seems like this could just be "div" :)
> > > > > 
> > > > > That has gathered some data now. Oddly enough, its poor performance
only shows up on the Android and Win bots. It doesn't behave poorly on the Linux
or Mac bots. No idea what's going on there. It runs at less than 1 run per
second for me locally.
> > > > > 
> > > > > OK to land?
> > > > 
> > > > I think we need to figure out your test case, first...
> > > > 
> > > > Here are my local results on today's canary:
> > > > values 2823.96234716872, 3167.9788801408263, 3003.9799734668595,
2873.295022732998, 2793.2960893854674, 3109.312604582613, 3254.644969033567,
2543.9660804522723, 262.6631644911388, 1.3286211569633022, 1837.33333333332,
1.3326847600834233, 1061.3262578249746, 1281.324791168054, 1069.319075745672,
227.99848001013186, 263.99824001173164, 218.6652088986081, 661.3156982480496,
817.3224357008531 runs/s
> > > > avg 1563.7181979156453 runs/s
> > > > median 1175.321933456863 runs/s
> > > > stdev 1247.151978828884 runs/s
> > > > min 1.3286211569633022 runs/s
> > > > max 3254.644969033567 runs/s
> > > > 
> > > > As low as 1.3, as high as 3,254!
> > > 
> > > Man.... reminding myself of the test... that makes no damn sense!
> > 
> > Making the test
> >         function test() {
> >             style.display = "block";
> >             PerfTestRunner.forceLayoutOrFullFrame();
> >             style.display = "none";
> >             PerfTestRunner.forceLayoutOrFullFrame();
> >         }
> > 
> > fixes it
> 
> OK to land this now?

From what I can see, results are pretty consistent just over 1 run/s now. Is
that what you see? LGTM if so.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405953005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405953005/60001

5 years, 1 month ago (2015-11-09 19:14:03 UTC) #14

commit-bot: I haz the power

5 years, 1 month ago (2015-11-09 20:38:34 UTC) #16

Message was sent while issue was closed.

Patchset 4 (id:??) landed as
https://crrev.com/74e95b29247f21620cb94fdf72e7e1051fff80c2
Cr-Commit-Position: refs/heads/master@{#358640}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages