Bug 74141 – REGRESSION(102234): 1% layout regression

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tony Chang]

I'll upload a small perf test that shows the regression.  On my machine, it's about 6ms (2-3%) slower at r102360 then at r102234.

[Tony Chang]

Created attachment 118493 [details]
perf test

[Tony Chang]

Also, I tried just commenting out the if() in RenderObject::style(), but it didn't seem to help.  Maybe there's something else going on.

[Mihnea Ovidenie]

I have a possible fix for it, testing.

[Mihnea Ovidenie]

Created attachment 118592 [details]
First attempt

[Mihnea Ovidenie]

I will make some more tests before converting into a patch. According to Tony, this patch seems to solve the regression.

[Tony Chang]

Created attachment 118655 [details]
Patch

[Dave Hyatt]

Comment on attachment 118655 [details]
Patch

r=me

[Tony Chang]

Committed r102479: <http://trac.webkit.org/changeset/102479>

[Tony Chang]

Hmm, still didn't seem to matter. Maybe region is not null there?

[Mihnea Ovidenie]

(In reply to comment #9)
> Hmm, still didn't seem to matter. Maybe region is not null there?

(In reply to comment #9)
> Hmm, still didn't seem to matter. Maybe region is not null there?

I doubt that the region is not null. In my first attempt, i took the test for region null outside initForRegionStyling function. In the patch, you moved the test up in the function (thus saving the unnecessary allocation) but the call to the non-inline function is still performed. 

When you say it does not matter, you mean that you are getting 2-3% regression in the performance test you uploaded to the bug or in the chrome test suite?

[Tony Chang]

(In reply to comment #10)
> I doubt that the region is not null. In my first attempt, i took the test for region null outside initForRegionStyling function. In the patch, you moved the test up in the function (thus saving the unnecessary allocation) but the call to the non-inline function is still performed. 

You're right, it's always NULL in when there are no regions.

> When you say it does not matter, you mean that you are getting 2-3% regression in the performance test you uploaded to the bug or in the chrome test suite?

They're the same.  I just took the chrome test suite and took the 2 pages that showed the biggest time difference to make the test I uploaded.

Another idea is that adding the bit to RuleData increased the size of the class by a byte (there are 33 bits) is causing problems.

[Tony Chang]

I commented out the 'if' line in RenderObject::style() on the perf bot and it went back down to it's previous speed:

http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/moz/report.html?history=150&rev=-1

Since we have to do the 'if' check, we'll have to do what Hyatt suggested on IRC: find the places where we call style() frequently in Render* classes and use a local variable instead of making lots of calls.

[Mihnea Ovidenie]

(In reply to comment #12)
> I commented out the 'if' line in RenderObject::style() on the perf bot and it went back down to it's previous speed:
> 
> http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/moz/report.html?history=150&rev=-1
> 
> Since we have to do the 'if' check, we'll have to do what Hyatt suggested on IRC: find the places where we call style() frequently in Render* classes and use a local variable instead of making lots of calls.

Ok, i am taking a look to see what can we do in this direction.

[Tony Chang]

http://build.chromium.org/f/chromium/perf/xp-release-dual-core/moz/report.html?history=210&rev=114220

You can see where we regressed and there is a small drop on the right due to reverting the change in RenderObject::style() (chromium r114195 incorporated this change).

I think we still need to do more.  Maybe we can inline part of initForRegionStyling.

[Tony Chang]

Even with inlining part of initForRegionStyling, there's a tiny .8%-1.5% regression depending on which set of HTML pages are used.  It only shows up on our Windows test bot.

You can use shift+click on the graph and the lower right will show the percent diff.

About 1% slow: http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/moz/report.html?history=250&rev=114451

Not noticeable change: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/moz/report.html?history=190&rev=114451

About 1.5% slow: http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/intl1/report.html?history=250&rev=114451

I'm going to make one more change to get rid of the call to initForRegionStyling to see if we can get back to our baseline.

[Tony Chang]

Removing the call to initForRegionStyling didn't change the perf results.  I'm going to roll it back in.

It's not clear to me if the remaining 1% was due to the original patch (r102234) or something else has crept in during the past week.  I guess we could try rolling out the original patch, but it might be easier at this point to just come up with the saving elsewhere.

This graph shows the regression most clearly:
http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/intl1/report.html?history=500&rev=114650

[Mihnea Ovidenie]

(In reply to comment #16)
> Removing the call to initForRegionStyling didn't change the perf results.  I'm going to roll it back in.
> 
> It's not clear to me if the remaining 1% was due to the original patch (r102234) or something else has crept in during the past week.  I guess we could try rolling out the original patch, but it might be easier at this point to just come up with the saving elsewhere.
> 
> This graph shows the regression most clearly:
> http://build.chromium.org/f/chromium/perf/vista-release-webkit-latest/intl1/report.html?history=500&rev=114650

Using a single call to RenderObject::style() shows some improvements on my machine. I want to use this way before rolling out the original patch.

[Tony Chang]

Since the original patch was rolled out, marking this as fixed.