Bug 76265

Summary: Cache RenderStyle pointer as a method to avoid performance regression for region styling
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: Layout and RenderingAssignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Normal CC: tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 76266    
Bug Blocks: 71488    
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Mihnea Ovidenie 2012-01-13 04:32:25 PST
During the investigation of regression introduced by region style patch (https://bugs.webkit.org/show_bug.cgi?id=71488), it turned out that an important contribution to the regression was introduced by the layout of RenderObject::style()

inline RenderStyle* RenderObject::style() const
{
    if (!inRenderFlowThread())
        return m_style.get();
    return styleInRegion();
}

During several runs of webkit build in instruments, i was able to identify some areas in which the method of caching RenderStyle pointer and replace style() with the cached pointer seems to fix the performance regression introduced by the modified version of style().

These areas are:
* updateBoxModelInfoFromStyle (RenderBoxModelObject/RenderBox)
* computePreferredLogicalWidths
* styleDidChange/styleWillChange (RenderText/RenderBlock/RenderBoxModelObject/RenderInline/RenderBox)
* RenderBlock layout methods (layoutRunsAndFloats/LineBreaker::nextLineBreak/layoutInlineChildren/layoutBlock)
* RenderBox willBeDestroyed/paintBoxDecorarations/computeRectForRepaint/computeLogicalHeight
* RenderText widthFromCache/width


I intend to create a WebKit bug for each of the above bullets instead of creating a big patch.
Comment 1 Mihnea Ovidenie 2012-01-13 10:23:57 PST
Created attachment 122448 [details]
Patch

I decided to add just one patch instead of multiple patches since this is mainly a mechanical replacement.
Comment 2 WebKit Review Bot 2012-01-13 10:26:39 PST
Attachment 122448 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderBox.cpp:2134:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebCore/rendering/RenderBlock.cpp:105:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/rendering/RenderBlock.cpp:112:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/rendering/RenderBlock.cpp:114:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:331:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:332:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:333:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:334:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:335:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:336:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:346:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:347:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 12 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Dave Hyatt 2012-01-18 11:22:04 PST
Comment on attachment 122448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122448&action=review

r=me with minor changes suggested.

> Source/WebCore/rendering/RenderBlock.cpp:247
> +    RenderStyle* newStyle = style();

This one isn't buying you anything. Only used once.

> Source/WebCore/rendering/RenderBlock.cpp:2049
> +    RenderStyle* cstyle = child->style();

Call this childStyle please, not cstyle.

> Source/WebCore/rendering/RenderBlock.cpp:2146
> +    if (cstyle->marginAfterCollapse() == MSEPARATE) {

childStyle not cstyle.
Comment 4 Mihnea Ovidenie 2012-01-18 23:48:30 PST
Created attachment 123082 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-01-19 01:23:16 PST
Comment on attachment 123082 [details]
Patch for landing

Clearing flags on attachment: 123082

Committed r105394: <http://trac.webkit.org/changeset/105394>
Comment 6 WebKit Review Bot 2012-01-19 01:23:21 PST
All reviewed patches have been landed.  Closing bug.