Bug 74141

Created attachment 118493 [details] perf test

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.

I have a possible fix for it, testing.

Created attachment 118592 [details] First attempt

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

Created attachment 118655 [details] Patch

Comment on attachment 118655 [details] Patch r=me

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

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

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

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.

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

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.

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.

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

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

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