Bug 113479 – logicalLeftSelectionGap and logicalRightSelectionGap call availableLogicalWidth() multiple times

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

We're calling availableLogicalWidth 4-5 times inside logicalLeftSelectionGap and logicalRightSelectionGap via logicalLeftOffsetForContent and logicalRightOffsetForContent:

Running Time	Self		Symbol Name
566.0ms    2.3%	0.0	 	 WebCore::RenderBoxModelObject::computedCSSPaddingLeft() const
195.0ms    0.8%	0.0	 	  WebCore::RenderBox::contentLogicalWidth() const
195.0ms    0.8%	0.0	 	   WebCore::RenderBlock::availableLogicalWidth() const
108.0ms    0.4%	0.0	 	    WebCore::RenderBlock::logicalRightOffsetForContent(WebCore::RenderRegion*, WebCore::LayoutUnit) const
106.0ms    0.4%	0.0	 	     WebCore::RenderBlock::logicalRightSelectionOffset(WebCore::RenderBlock*, WebCore::LayoutUnit)
29.0ms    0.1%	0.0	 	      WebCore::RenderBlock::logicalRightSelectionGap(WebCore::RenderBlock*, WebCore::LayoutPoint const&, WebCore::LayoutSize const&, WebCore::RenderObject*, WebCore::LayoutUnit, WebCore::LayoutUnit, WebCore::LayoutUnit, WebCore::PaintInfo const*)
26.0ms    0.1%	0.0	 	       WebCore::RootInlineBox::lineSelectionGap(WebCore::RenderBlock*, WebCore::LayoutPoint const&, WebCore::LayoutSize const&, WebCore::LayoutUnit, WebCore::LayoutUnit, WebCore::PaintInfo const*)
26.0ms    0.1%	0.0	 	        WebCore::RenderBlock::inlineSelectionGaps(WebCore::RenderBlock*, WebCore::LayoutPoint const&, WebCore::LayoutSize const&, WebCore::LayoutUnit&, WebCore::LayoutUnit&, WebCore::LayoutUnit&, WebCore::PaintInfo const*)
26.0ms    0.1%	0.0	 	         WebCore::RenderBlock::selectionGaps(WebCore::RenderBlock*, WebCore::LayoutPoint const&, WebCore::LayoutSize const&, WebCore::LayoutUnit&, WebCore::LayoutUnit&, WebCore::LayoutUnit&, WebCore::PaintInfo const*)
24.0ms    0.0%	0.0	 	          WebCore::RenderBlock::blockSelectionGaps(WebCore::RenderBlock*, WebCore::LayoutPoint const&, WebCore::LayoutSize const&, WebCore::LayoutUnit&, WebCore::LayoutUnit&, WebCore::LayoutUnit&, WebCore::PaintInfo const*)
24.0ms    0.0%	0.0	 	           WebCore::RenderBlock::selectionGaps(WebCore::RenderBlock*, WebCore::LayoutPoint const&, WebCore::LayoutSize const&, WebCore::LayoutUnit&, WebCore::LayoutUnit&, WebCore::LayoutUnit&, WebCore::PaintInfo const*)
2.0ms    0.0%	0.0	 	          WebCore::RenderBlock::selectionGapRectsForRepaint(WebCore::RenderLayerModelObject const*)

Obviously, we should be caching the value.

[Radar WebKit Bug Importer]

<rdar://problem/13523749>

[Ryosuke Niwa]

Created attachment 195486 [details]
Fixes the bug

[WebKit Review Bot]

Attachment 195486 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h']" exit_code: 1
Source/WebCore/rendering/RenderBlock.cpp:3714:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/rendering/RenderBlock.cpp:3732:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 195487 [details]
A/B test result

[Ryosuke Niwa]

With the improvement I made in the bug 113364, we're looking at 36-38% improvement.

[Ryosuke Niwa]

Comment on attachment 195486 [details]
Fixes the bug

Oops, this is a bad patch.

[Ryosuke Niwa]

Created attachment 198790 [details]
Work in progress

[Ryosuke Niwa]

Created attachment 198792 [details]
WIP A/B test

[Ryosuke Niwa]

Created attachment 199129 [details]
Work in progress 2

[Ryosuke Niwa]

Created attachment 199143 [details]
Optimizes the code

[Ryosuke Niwa]

Created attachment 199144 [details]
A/B test result

A crude A/B testing reveals that this is a roughly 20% performance improvement.

[Build Bot]

Comment on attachment 199143 [details]
Optimizes the code

Attachment 199143 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/110762

New failing tests:
fast/repaint/japanese-rl-selection-repaint-in-regions.html

[Build Bot]

Created attachment 199158 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 199143 [details])
> Attachment 199143 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/110762
> 
> New failing tests:
> fast/repaint/japanese-rl-selection-repaint-in-regions.html

I don't think this is a real failure. The test is just flaky :(
http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Frepaint%2Fjapanese-rl-selection-repaint-in-regions.html

e.g. the same failure was observed on 
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r148937%20(8237)/results.html

[Ryosuke Niwa]

Created attachment 199161 [details]
A/B/ test result with r147007

This A/B testing confirms that we have an overall improvement of 36-37% when combined with http://trac.webkit.org/changeset/147008.

[Dave Hyatt]

Comment on attachment 199143 [details]
Optimizes the code

r=me

[WebKit Commit Bot]

Comment on attachment 199143 [details]
Optimizes the code

Rejecting attachment 199143 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
mal/x86_64/RenderedPosition.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/editing/RenderedPosition.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/RenderedPosition.o


** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/RenderBlock.o rendering/RenderBlock.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.appspot.com/results/22367

[Ryosuke Niwa]

Committed r149007: <http://trac.webkit.org/changeset/149007>