Commit 7f28b61b authored by robhogan@gmail.com's avatar robhogan@gmail.com

Do not cache the result of RenderStyle::computedLineHeight()

We currently use 26-bits in RenderBlock to cache the result of
RenderStyle::computedLineHeight(). However when you look at the
computations behind that function the main components are already cached
so at most we are saving a handful of computations per call.

In https://codereview.chromium.org/256593009/ we added a test that calls
RenderBlock::lineHeight() ~425,000 times on each run. This CL results in
no measurable degradation in performance against that test.

So since we cannot get a measurable improvement in performance as a result
of the caching when testing locally, remove it and free up space for more useful
bits in RenderBlock. We will let it cycle through the perf bots for a while
before allowing people to start using the spare bits though.

The cached result originally sat in RenderText and was inherited from the
fork from KHTML. http://trac.webkit.org/changeset/6010 moved it to RenderBlock
on the basis that "calls to fontMetrics().lineSpacing() are expensive." This
is no longer the case as far as I can tell, since both lineSpacing() and
fontMetrics() are now cached themselves.

Review URL: https://codereview.chromium.org/260073005

git-svn-id: svn://svn.chromium.org/blink/trunk@176233 bbb929c8-8fbe-4397-9dbb-9b2b20218538
parent 1f0843d3
...@@ -970,8 +970,6 @@ void FastTextAutosizer::applyMultiplier(RenderObject* renderer, float multiplier ...@@ -970,8 +970,6 @@ void FastTextAutosizer::applyMultiplier(RenderObject* renderer, float multiplier
renderer->setStyleInternal(style.release()); renderer->setStyleInternal(style.release());
renderer->setNeedsLayoutAndFullPaintInvalidation(); renderer->setNeedsLayoutAndFullPaintInvalidation();
if (renderer->isRenderBlock())
toRenderBlock(renderer)->invalidateLineHeight();
break; break;
case LayoutNeeded: case LayoutNeeded:
......
...@@ -35,7 +35,6 @@ static PassRefPtr<StringImpl> newlineString() ...@@ -35,7 +35,6 @@ static PassRefPtr<StringImpl> newlineString()
RenderBR::RenderBR(Node* node) RenderBR::RenderBR(Node* node)
: RenderText(node, newlineString()) : RenderText(node, newlineString())
, m_lineHeight(-1)
{ {
} }
...@@ -45,22 +44,13 @@ RenderBR::~RenderBR() ...@@ -45,22 +44,13 @@ RenderBR::~RenderBR()
int RenderBR::lineHeight(bool firstLine) const int RenderBR::lineHeight(bool firstLine) const
{ {
if (firstLine && document().styleEngine()->usesFirstLineRules()) { RenderStyle* s = style(firstLine && document().styleEngine()->usesFirstLineRules());
RenderStyle* s = style(firstLine); return s->computedLineHeight();
if (s != style())
return s->computedLineHeight();
}
if (m_lineHeight == -1)
m_lineHeight = style()->computedLineHeight();
return m_lineHeight;
} }
void RenderBR::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle) void RenderBR::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
{ {
RenderText::styleDidChange(diff, oldStyle); RenderText::styleDidChange(diff, oldStyle);
m_lineHeight = -1;
} }
int RenderBR::caretMinOffset() const int RenderBR::caretMinOffset() const
......
...@@ -55,9 +55,6 @@ public: ...@@ -55,9 +55,6 @@ public:
protected: protected:
virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle) OVERRIDE; virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle) OVERRIDE;
private:
mutable int m_lineHeight;
}; };
DEFINE_RENDER_OBJECT_TYPE_CASTS(RenderBR, isBR()); DEFINE_RENDER_OBJECT_TYPE_CASTS(RenderBR, isBR());
......
...@@ -155,7 +155,6 @@ private: ...@@ -155,7 +155,6 @@ private:
RenderBlock::RenderBlock(ContainerNode* node) RenderBlock::RenderBlock(ContainerNode* node)
: RenderBox(node) : RenderBox(node)
, m_lineHeight(-1)
, m_hasMarginBeforeQuirk(false) , m_hasMarginBeforeQuirk(false)
, m_hasMarginAfterQuirk(false) , m_hasMarginAfterQuirk(false)
, m_beingDestroyed(false) , m_beingDestroyed(false)
...@@ -351,7 +350,6 @@ void RenderBlock::styleDidChange(StyleDifference diff, const RenderStyle* oldSty ...@@ -351,7 +350,6 @@ void RenderBlock::styleDidChange(StyleDifference diff, const RenderStyle* oldSty
textAutosizer->record(this); textAutosizer->record(this);
propagateStyleToAnonymousChildren(true); propagateStyleToAnonymousChildren(true);
invalidateLineHeight();
// It's possible for our border/padding to change, but for the overall logical width of the block to // It's possible for our border/padding to change, but for the overall logical width of the block to
// end up being the same. We keep track of this change so in layoutBlock, we can know to set relayoutChildren=true. // end up being the same. We keep track of this change so in layoutBlock, we can know to set relayoutChildren=true.
...@@ -3784,16 +3782,8 @@ LayoutUnit RenderBlock::lineHeight(bool firstLine, LineDirectionMode direction, ...@@ -3784,16 +3782,8 @@ LayoutUnit RenderBlock::lineHeight(bool firstLine, LineDirectionMode direction,
if (isReplaced() && linePositionMode == PositionOnContainingLine) if (isReplaced() && linePositionMode == PositionOnContainingLine)
return RenderBox::lineHeight(firstLine, direction, linePositionMode); return RenderBox::lineHeight(firstLine, direction, linePositionMode);
if (firstLine && document().styleEngine()->usesFirstLineRules()) { RenderStyle* s = style(firstLine && document().styleEngine()->usesFirstLineRules());
RenderStyle* s = style(firstLine); return s->computedLineHeight();
if (s != style())
return s->computedLineHeight();
}
if (m_lineHeight == -1)
m_lineHeight = style()->computedLineHeight();
return m_lineHeight;
} }
int RenderBlock::beforeMarginInLineDirection(LineDirectionMode direction) const int RenderBlock::beforeMarginInLineDirection(LineDirectionMode direction) const
......
...@@ -505,8 +505,6 @@ protected: ...@@ -505,8 +505,6 @@ protected:
public: public:
virtual LayoutUnit offsetFromLogicalTopOfFirstPage() const OVERRIDE FINAL; virtual LayoutUnit offsetFromLogicalTopOfFirstPage() const OVERRIDE FINAL;
void invalidateLineHeight() { m_lineHeight = -1; }
public: public:
// Allocated only when some of these fields have non-default values // Allocated only when some of these fields have non-default values
...@@ -534,7 +532,7 @@ protected: ...@@ -534,7 +532,7 @@ protected:
RenderObjectChildList m_children; RenderObjectChildList m_children;
RenderLineBoxList m_lineBoxes; // All of the root line boxes created for this block flow. For example, <div>Hello<br>world.</div> will have two total lines for the <div>. RenderLineBoxList m_lineBoxes; // All of the root line boxes created for this block flow. For example, <div>Hello<br>world.</div> will have two total lines for the <div>.
mutable signed m_lineHeight : 26; // WARNING: Don't add any bits here until we are comfortable that removing m_lineHeight has not regressed performance. See http://crrev.com/260073005 for more information.
unsigned m_hasMarginBeforeQuirk : 1; // Note these quirk values can't be put in RenderBlockRareData since they are set too frequently. unsigned m_hasMarginBeforeQuirk : 1; // Note these quirk values can't be put in RenderBlockRareData since they are set too frequently.
unsigned m_hasMarginAfterQuirk : 1; unsigned m_hasMarginAfterQuirk : 1;
unsigned m_beingDestroyed : 1; unsigned m_beingDestroyed : 1;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment