Optimize for horizontal writing mode

Optimize for horizontal writing mode

Optimize layout and painting by avoiding or short-circuiting vertical
writing mode logic unless it is used at least once in a document.

A per-document containsAnyVerticalWritingModes flag is introduced that
is set the first time a vertical writing mode is encountered. Unless
this flag has been set all flipForWritingMode functions short-circuit.
Some of the RenderBox flip methods is moved to the header file to allow
for better inlining.

Early testing indicates a ~10% performance improvement across the board
with 35 of our 42 layout performance tests showing a 6% to 19%
performance improvement. The remaining 7 tests show no significant
improvement and none of them show any regression.

R=dglazkov@chromium.org, pdr@chromium.org
BUG=425364
TEST=PerformanceTests/Layout

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184217

[eae, 2014-10-20 23:15:56]

eae@chromium.org changed reviewers:
+ chrishtr@chromium.org

[eae, 2014-10-21 00:12:40]

eae@chromium.org changed reviewers:
+ pdr@chromium.org

[eae, 2014-10-21 21:18:58]

eae@chromium.org changed reviewers:
+ dglazkov@chromium.org
- chrishtr@chromium.org

[eae, 2014-10-21 21:19:52]

Ready for review.

Not quite sure that the Document itself is the right place for the flag but it
needs to be something of about that scope.

[pdr., 2014-10-21 22:11:26]

This is incredible!

https://codereview.chromium.org/669803002/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/669803002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:785: bool hasVerticalWritingMode() const { return
m_hasVerticalWritingMode; }
Would we still see the perf benefit if we moved the unlikely in here? This would
be a bit cleaner and I think it'll all inline.

Bikeshed: I can see someone not realizing that this is for the entire document.
What do you think about containsAnyVerticalWritingModes or similar?

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InlineBox.cpp (right):

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineBox.cpp:324: if
(!UNLIKELY(renderer().document().hasVerticalWritingMode())
I'm not a huge fan of sprinkling unlikely's around because we may miss out on
wins both in this patch and in future refactorings. What do you think about
changing RenderStyle::isFlippedBlocksWritingMode to
RenderStyle::slowIsFlippedBlocksWritingMode and introducing
RenderObject::hasFlippedBlocksWritingMode?

I don't feel strongly about this, but De Morgan's might make this line easier to
read.
(!x || !y) -> (!(x && y))

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:540: LayoutUnit flipForWritingMode(LayoutUnit
position) const
Is it possible to separate the inlining of these so we can measure the perf
impact of this change and the inlining separately?

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2252: static inline LayoutRect
flippedLogicalBoundingBox(LayoutRect boundingBox, RenderObject* renderer)
I don't understand the purpose of making this a static.

[eae, 2014-10-21 22:21:46]

https://codereview.chromium.org/669803002/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/669803002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:785: bool hasVerticalWritingMode() const { return
m_hasVerticalWritingMode; }
On 2014/10/21 22:11:26, pdr wrote:
> Would we still see the perf benefit if we moved the unlikely in here? This
would
> be a bit cleaner and I think it'll all inline.

The effect of the UNLIKELY macro is minimal, it didn't show a statistically
significant improvement on any of the tests. I'm guessing it adds 0.1-0.5% by
priming the branch predictor. The macro only works for conditions (afaik) so I'm
not sure how it could be used here.

> Bikeshed: I can see someone not realizing that this is for the entire
document.
> What do you think about containsAnyVerticalWritingModes or similar?

That is a much better name, thanks!

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InlineBox.cpp (right):

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineBox.cpp:324: if
(!UNLIKELY(renderer().document().hasVerticalWritingMode())
On 2014/10/21 22:11:26, pdr wrote:
> I'm not a huge fan of sprinkling unlikely's around because we may miss out on
> wins both in this patch and in future refactorings. What do you think about
> changing RenderStyle::isFlippedBlocksWritingMode to
> RenderStyle::slowIsFlippedBlocksWritingMode and introducing
> RenderObject::hasFlippedBlocksWritingMode?
> 
> I don't feel strongly about this, but De Morgan's might make this line easier
to
> read.
> (!x || !y) -> (!(x && y))

The entire point of this change is to short-circuit the conditions as early as
possible, changing the condition would require the second part to always be
evaluated negating much of the improvement. I do agree that it would make it
more readable though.

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:540: LayoutUnit flipForWritingMode(LayoutUnit
position) const
On 2014/10/21 22:11:26, pdr wrote:
> Is it possible to separate the inlining of these so we can measure the perf
> impact of this change and the inlining separately?

Inlining adds about another 2-3% (see the second and third runs in the perf
results I shared with you). Inlining on its own (without the document level
flag) showed less than 1% improvement.

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2252: static inline LayoutRect
flippedLogicalBoundingBox(LayoutRect boundingBox, RenderObject* renderer)
On 2014/10/21 22:11:26, pdr wrote:
> I don't understand the purpose of making this a static.

Allows it to be inlined easier. It is a very hot function.

[pdr., 2014-10-21 22:38:15]

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InlineBox.cpp (right):

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InlineBox.cpp:324: if
(!UNLIKELY(renderer().document().hasVerticalWritingMode())
On 2014/10/21 at 22:21:46, eae wrote:
> On 2014/10/21 22:11:26, pdr wrote:
> > I'm not a huge fan of sprinkling unlikely's around because we may miss out
on
> > wins both in this patch and in future refactorings. What do you think about
> > changing RenderStyle::isFlippedBlocksWritingMode to
> > RenderStyle::slowIsFlippedBlocksWritingMode and introducing
> > RenderObject::hasFlippedBlocksWritingMode?
> > 
> > I don't feel strongly about this, but De Morgan's might make this line
easier to
> > read.
> > (!x || !y) -> (!(x && y))
> 
> The entire point of this change is to short-circuit the conditions as early as
possible, changing the condition would require the second part to always be
evaluated negating much of the improvement. I do agree that it would make it
more readable though.

What do you think about the first part involving moving this code onto
RenderObject?

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2252: static inline LayoutRect
flippedLogicalBoundingBox(LayoutRect boundingBox, RenderObject* renderer)
On 2014/10/21 at 22:21:46, eae wrote:
> On 2014/10/21 22:11:26, pdr wrote:
> > I don't understand the purpose of making this a static.
> 
> Allows it to be inlined easier. It is a very hot function.

Could we just make this inline then?

[eae, 2014-10-21 22:41:21]

On 2014/10/21 22:38:15, pdr wrote:
>
https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/In...
> File Source/core/rendering/InlineBox.cpp (right):
> 
>
https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/In...
> Source/core/rendering/InlineBox.cpp:324: if
> (!UNLIKELY(renderer().document().hasVerticalWritingMode())
> On 2014/10/21 at 22:21:46, eae wrote:
> > On 2014/10/21 22:11:26, pdr wrote:
> > > I'm not a huge fan of sprinkling unlikely's around because we may miss out
> on
> > > wins both in this patch and in future refactorings. What do you think
about
> > > changing RenderStyle::isFlippedBlocksWritingMode to
> > > RenderStyle::slowIsFlippedBlocksWritingMode and introducing
> > > RenderObject::hasFlippedBlocksWritingMode?
> > > 
> > > I don't feel strongly about this, but De Morgan's might make this line
> easier to
> > > read.
> > > (!x || !y) -> (!(x && y))
> > 
> > The entire point of this change is to short-circuit the conditions as early
as
> possible, changing the condition would require the second part to always be
> evaluated negating much of the improvement. I do agree that it would make it
> more readable though.
> 
> What do you think about the first part involving moving this code onto
> RenderObject?

Oh, interesting. Let me try that!

> 
>
https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/669803002/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayer.cpp:2252: static inline LayoutRect
> flippedLogicalBoundingBox(LayoutRect boundingBox, RenderObject* renderer)
> On 2014/10/21 at 22:21:46, eae wrote:
> > On 2014/10/21 22:11:26, pdr wrote:
> > > I don't understand the purpose of making this a static.
> > 
> > Allows it to be inlined easier. It is a very hot function.
> 
> Could we just make this inline then?

That's what I did... Having it be a class function and moving the implementation
to the header would work to but would require a bunch of extra includes for the
header.

[eae, 2014-10-21 23:16:37]

Moving the logic into RenderBlock as suggested, had no negative performance
impact and cleaned up the code quite a bit. Thanks for the suggestion.

PTAL.

[leviw_travelin_and_unemployed, 2014-10-21 23:29:59]

leviw@chromium.org changed reviewers:
+ leviw@chromium.org

[leviw_travelin_and_unemployed, 2014-10-21 23:30:00]

https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/In...
File Source/core/rendering/InlineBox.cpp (right):

https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/In...
Source/core/rendering/InlineBox.cpp:321: void
InlineBox::flipForWritingMode(FloatRect& rect)
I wonder how much of a difference inlining these would make.

https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:540: LayoutUnit flipForWritingMode(LayoutUnit
position) const
I'm curious how much of this win is simply due to this inlining. Seems like that
alone could make a big difference.

[eae, 2014-10-21 23:32:22]

On 2014/10/21 23:30:00, leviw wrote:
>
https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/In...
> File Source/core/rendering/InlineBox.cpp (right):
> 
>
https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/In...
> Source/core/rendering/InlineBox.cpp:321: void
> InlineBox::flipForWritingMode(FloatRect& rect)
> I wonder how much of a difference inlining these would make.

I could give it a try and see if you'd like.


>
https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderBox.h (right):
> 
>
https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderBox.h:540: LayoutUnit
flipForWritingMode(LayoutUnit
> position) const
> I'm curious how much of this win is simply due to this inlining. Seems like
that
> alone could make a big difference.

Inlining added an extra 2-3 percentage points on top of the other change.
Inlining without the global flag only improved performance by about 1%.

[leviw_travelin_and_unemployed, 2014-10-21 23:35:16]

On 2014/10/21 at 23:32:22, eae wrote:
> On 2014/10/21 23:30:00, leviw wrote:
> >
https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/In...
> > File Source/core/rendering/InlineBox.cpp (right):
> > 
> >
https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/In...
> > Source/core/rendering/InlineBox.cpp:321: void
> > InlineBox::flipForWritingMode(FloatRect& rect)
> > I wonder how much of a difference inlining these would make.
> 
> I could give it a try and see if you'd like.
Not in this patch. Could make for a potential follow-up if it's also a win :)

> 
> 
> >
https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/Re...
> > File Source/core/rendering/RenderBox.h (right):
> > 
> >
https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/Re...
> > Source/core/rendering/RenderBox.h:540: LayoutUnit
flipForWritingMode(LayoutUnit
> > position) const
> > I'm curious how much of this win is simply due to this inlining. Seems like
that
> > alone could make a big difference.
> 
> Inlining added an extra 2-3 percentage points on top of the other change.
Inlining without the global flag only improved performance by about 1%.

Amazing.

[pdr., 2014-10-21 23:35:32]

pdr@chromium.org changed reviewers:
- leviw@chromium.org

[pdr., 2014-10-21 23:35:34]

A few more minor comments.

https://codereview.chromium.org/669803002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/669803002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:2880: return
renderView->style()->isFlippedBlocksWritingMode();
Why not renderView->hasFlippedBlocksWritingMode()? (here, and elsewhere).

https://codereview.chromium.org/669803002/diff/60001/Source/core/paint/BlockP...
File Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/669803002/diff/60001/Source/core/paint/BlockP...
Source/core/paint/BlockPainter.cpp:114: &&
renderer.style()->slowIsFlippedBlocksWritingMode()) { // Faster than calling
containingBlock().
I think this comment belongs with the parent() call on line 112.

https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/In...
File Source/core/rendering/InlineBox.cpp (right):

https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/In...
Source/core/rendering/InlineBox.cpp:311: if
(!UNLIKELY(renderer().hasFlippedBlocksWritingMode()))
Because hasFlippedBlocksWritingMode will be inlined here, can we move the
unlikely inside hasFlippedBlocksWritingMode?

[eae, 2014-10-21 23:39:42]

On 2014/10/21 23:35:34, pdr wrote:
> A few more minor comments.
> 
>
https://codereview.chromium.org/669803002/diff/60001/Source/core/frame/FrameV...
> File Source/core/frame/FrameView.cpp (left):
> 
>
https://codereview.chromium.org/669803002/diff/60001/Source/core/frame/FrameV...
> Source/core/frame/FrameView.cpp:2880: return
> renderView->style()->isFlippedBlocksWritingMode();
> Why not renderView->hasFlippedBlocksWritingMode()? (here, and elsewhere).

Good idea.

> 
>
https://codereview.chromium.org/669803002/diff/60001/Source/core/paint/BlockP...
> File Source/core/paint/BlockPainter.cpp (right):
> 
>
https://codereview.chromium.org/669803002/diff/60001/Source/core/paint/BlockP...
> Source/core/paint/BlockPainter.cpp:114: &&
> renderer.style()->slowIsFlippedBlocksWritingMode()) { // Faster than calling
> containingBlock().
> I think this comment belongs with the parent() call on line 112.
> 
>
https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/In...
> File Source/core/rendering/InlineBox.cpp (right):
> 
>
https://codereview.chromium.org/669803002/diff/60001/Source/core/rendering/In...
> Source/core/rendering/InlineBox.cpp:311: if
> (!UNLIKELY(renderer().hasFlippedBlocksWritingMode()))
> Because hasFlippedBlocksWritingMode will be inlined here, can we move the
> unlikely inside hasFlippedBlocksWritingMode?

Does that work? I was under the distinct impression that it only worked when
used in conditions. We don't have a single use of the macro that is not in a if
or while statement.

[eae, 2014-10-21 23:44:35]

Changed it to use RenderView to use hasFlippedBlocksWritingMode and moved
comment as suggested. Did not move the UNLIKELY macro.

[eae, 2014-10-22 00:40:58]

Hmm, one of the changes between patchset 2 and 5 introduced a bug breaking some
writing mode tests

[eae, 2014-10-22 00:41:19]

Hmm, one of the changes between patchset 2 and 5 introduced a bug breaking some
writing mode tests

[pdr., 2014-10-22 18:41:58]

LGTM % compile issue.

@leviw, could you please add your blessing as well?

https://codereview.chromium.org/669803002/diff/80001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/669803002/diff/80001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:906: direction,
curBox->isHorizontalWritingMode(),
curBox->style()->slowIsFlippedBlocksWritingMode());
Nearly all of these should be able to use the faster version.

[pdr., 2014-10-22 18:42:54]

https://codereview.chromium.org/669803002/diff/100001/Source/core/page/PrintC...
File Source/core/page/PrintContext.cpp (right):

https://codereview.chromium.org/669803002/diff/100001/Source/core/page/PrintC...
Source/core/page/PrintContext.cpp:126: if
(view->style()->slowIsFlippedBlocksWritingMode()) {
(We'll be doing these in a followup)

[leviw_travelin_and_unemployed, 2014-10-22 18:50:25]

leviw@chromium.org changed reviewers:
+ leviw@chromium.org

[leviw_travelin_and_unemployed, 2014-10-22 18:50:27]

lgtm

https://codereview.chromium.org/669803002/diff/80001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/669803002/diff/80001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:906: direction,
curBox->isHorizontalWritingMode(),
curBox->style()->slowIsFlippedBlocksWritingMode());
On 2014/10/22 18:41:58, pdr wrote:
> Nearly all of these should be able to use the faster version.

I'm fine with doing it in a follow-up as long as we do it. Event handling is
also perf-sensitive.

[eae, 2014-10-22 19:39:26]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-10-22 19:40:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/669803002/120001

[commit-bot: I haz the power, 2014-10-22 20:17:59]

Committed patchset #7 (id:120001) as 184217

[chrishtr, 2014-10-28 18:18:20]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2014-10-28 18:18:21]

Cool.

https://codereview.chromium.org/669803002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/669803002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderBox.cpp:3237: if
(containerBlock->style()->slowIsFlippedBlocksWritingMode() &&
child->isHorizontalWritingMode() == containerBlock->isHorizontalWritingMode()) {
Are cases like this ones that didn't make the metric faster?

[pdr., 2014-10-28 18:32:28]

On 2014/10/28 at 18:18:21, chrishtr wrote:
> Cool.
> 
>
https://codereview.chromium.org/669803002/diff/120001/Source/core/rendering/R...
> File Source/core/rendering/RenderBox.cpp (right):
> 
>
https://codereview.chromium.org/669803002/diff/120001/Source/core/rendering/R...
> Source/core/rendering/RenderBox.cpp:3237: if
(containerBlock->style()->slowIsFlippedBlocksWritingMode() &&
child->isHorizontalWritingMode() == containerBlock->isHorizontalWritingMode()) {
> Are cases like this ones that didn't make the metric faster?

Unfortunately this patch didn't show the perf wins we had hoped for :( Emil's
numbers were correct on his machine but for some reason they didn't translate to
perf wins on the bots.

Emil's out this week but I think the plan is to roll this out and dig back into
it next week.