Bug 113479 - logicalLeftSelectionGap and logicalRightSelectionGap call availableLogicalWidth() multiple times
Summary: logicalLeftSelectionGap and logicalRightSelectionGap call availableLogicalWid...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on: 114853
Blocks: 113478 117408
  Show dependency treegraph
 
Reported: 2013-03-27 22:40 PDT by Ryosuke Niwa
Modified: 2013-06-20 21:53 PDT (History)
15 users (show)

See Also:


Attachments
Fixes the bug (16.18 KB, patch)
2013-03-27 22:52 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
A/B test result (26.82 KB, text/html)
2013-03-27 23:01 PDT, Ryosuke Niwa
no flags Details
Work in progress (8.42 KB, patch)
2013-04-18 20:12 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP A/B test (26.73 KB, text/html)
2013-04-18 20:54 PDT, Ryosuke Niwa
no flags Details
Work in progress 2 (39.25 KB, patch)
2013-04-22 16:33 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Optimizes the code (50.26 KB, patch)
2013-04-22 21:19 PDT, Ryosuke Niwa
hyatt: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
A/B test result (31.16 KB, text/html)
2013-04-22 21:21 PDT, Ryosuke Niwa
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (805.10 KB, application/zip)
2013-04-22 23:50 PDT, Build Bot
no flags Details
A/B/ test result with r147007 (24.76 KB, text/html)
2013-04-23 00:05 PDT, Ryosuke Niwa
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-03-27 22:40:28 PDT
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.
Comment 1 Radar WebKit Bug Importer 2013-03-27 22:50:40 PDT
<rdar://problem/13523749>
Comment 2 Ryosuke Niwa 2013-03-27 22:52:59 PDT
Created attachment 195486 [details]
Fixes the bug
Comment 3 WebKit Review Bot 2013-03-27 22:54:28 PDT
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.
Comment 4 Ryosuke Niwa 2013-03-27 23:01:29 PDT
Created attachment 195487 [details]
A/B test result
Comment 5 Ryosuke Niwa 2013-03-27 23:07:44 PDT
With the improvement I made in the bug 113364, we're looking at 36-38% improvement.
Comment 6 Ryosuke Niwa 2013-03-27 23:26:08 PDT
Comment on attachment 195486 [details]
Fixes the bug

Oops, this is a bad patch.
Comment 7 Ryosuke Niwa 2013-04-18 20:12:01 PDT
Created attachment 198790 [details]
Work in progress
Comment 8 Ryosuke Niwa 2013-04-18 20:54:04 PDT
Created attachment 198792 [details]
WIP A/B test
Comment 9 Ryosuke Niwa 2013-04-22 16:33:44 PDT
Created attachment 199129 [details]
Work in progress 2
Comment 10 Ryosuke Niwa 2013-04-22 21:19:54 PDT
Created attachment 199143 [details]
Optimizes the code
Comment 11 Ryosuke Niwa 2013-04-22 21:21:26 PDT
Created attachment 199144 [details]
A/B test result

A crude A/B testing reveals that this is a roughly 20% performance improvement.
Comment 12 Build Bot 2013-04-22 23:50:05 PDT
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
Comment 13 Build Bot 2013-04-22 23:50:07 PDT
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
Comment 14 Ryosuke Niwa 2013-04-22 23:55:40 PDT
(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
Comment 15 Ryosuke Niwa 2013-04-23 00:05:25 PDT
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.
Comment 16 Dave Hyatt 2013-04-23 11:49:39 PDT
Comment on attachment 199143 [details]
Optimizes the code

r=me
Comment 17 WebKit Commit Bot 2013-04-23 12:30:36 PDT
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
Comment 18 Ryosuke Niwa 2013-04-23 17:17:39 PDT
Committed r149007: <http://trac.webkit.org/changeset/149007>