Optimize SVGTextQuery's character offset ligature adjustment query

Source/core/rendering/svg/SVGTextQuery.cpp

Index: Source/core/rendering/svg/SVGTextQuery.cpp
diff --git a/Source/core/rendering/svg/SVGTextQuery.cpp b/Source/core/rendering/svg/SVGTextQuery.cpp
index f5ead0d43661f2ec0f636f48021b511100a0b748..7b9fcb95171fa59a0034ff11510e4f1cbacf750b 100644
--- a/Source/core/rendering/svg/SVGTextQuery.cpp
+++ b/Source/core/rendering/svg/SVGTextQuery.cpp
@@ -38,13 +38,36 @@ struct SVGTextQuery::Data {
         , processedCharacters(0)
         , textRenderer(0)
         , textBox(0)
+        , ligatureAdjustmentLastStartPositionQuery(-1)
+        , ligatureAdjustmentLastEndPositionQuery(-1)
+        , ligatureAdjustmentLastStartPositionResult(-1)
+        , ligatureAdjustmentLastEndPositionResult(-1)
     {
     }
 
+    void setupForTextBox(const SVGInlineTextBox* newTextBox)
+    {
+        textBox = newTextBox;
+        textRenderer = &toRenderSVGInlineText(textBox->textRenderer());
+        ASSERT(textRenderer->style());
+        ASSERT(textRenderer->style()->svgStyle());
+
+        isVerticalText = textRenderer->style()->svgStyle()->isVerticalWritingMode();
+
+        // Invalidate the ligature-adjustment cache.

[pdr., 2014/06/24 21:18:48]

Is it possible to invalidate the cache when it really becomes invalid (when
start/end Position or processedCharacters changes) instead of doing it
explicitly here?

[fs, 2014/06/25 10:29:49]

That pretty much what it does ATM - in the loop in executeQuery(),
processedCharacters is set at the end up the loop-body, and this is performed at
the start of the loop-body. Start/end positions will be invariant in the cases
where this matter - _except_ the positions queried are first adjusted by
processedCharacters (i.e. they're dependent.)

+        ligatureAdjustmentLastStartPositionQuery = ligatureAdjustmentLastEndPositionQuery = -1;
+        ligatureAdjustmentLastStartPositionResult = ligatureAdjustmentLastEndPositionResult = -1;
+    }
+
     bool isVerticalText;
     unsigned processedCharacters;
     RenderSVGInlineText* textRenderer;
     const SVGInlineTextBox* textBox;
+    // Ligature adjustment cache.

[pdr., 2014/06/24 21:18:48]

The cache key is pretty much {startPosition, processedCharacters} so I think it
would be better to put this on SVGTextQuery instead of Data.

[fs, 2014/06/25 10:29:49]

I suppose there could be some merit to that... To me it seems though that
keeping it as Data would enable further changes that would better clarify which
state is required by which query (not all queries use this cache after all). As
an example we might have "CharacterRangeQueryData" (derived from Data), that
contains "position", "length" and the cache/state used for adjustment.
Somewhat on a tangent, but it also feels a bit wrong to add additional state to
something that only has redundant state ATM (getting rid of m_textBoxes ought to
be trivial) =). An on the same note, it seems kind of redundant with all the
"double this" going on (since SVGTextQuery is stateless, and Data is what's
actually used).

But, if none of the above tickles your fancy I can move the cache state.

+    int ligatureAdjustmentLastStartPositionQuery;
+    int ligatureAdjustmentLastEndPositionQuery;
+    int ligatureAdjustmentLastStartPositionResult;
+    int ligatureAdjustmentLastEndPositionResult;
 };
 
 static inline InlineFlowBox* flowBoxForRenderer(RenderObject* renderer)
@@ -111,12 +134,8 @@ bool SVGTextQuery::executeQuery(Data* queryData, ProcessTextFragmentCallback fra
 
     // Loop over all text boxes
     for (unsigned textBoxPosition = 0; textBoxPosition < textBoxCount; ++textBoxPosition) {
-        queryData->textBox = m_textBoxes.at(textBoxPosition);
-        queryData->textRenderer = &toRenderSVGInlineText(queryData->textBox->textRenderer());
-        ASSERT(queryData->textRenderer->style());
-        ASSERT(queryData->textRenderer->style()->svgStyle());
+        queryData->setupForTextBox(m_textBoxes.at(textBoxPosition));
 
-        queryData->isVerticalText = queryData->textRenderer->style()->svgStyle()->isVerticalWritingMode();
         const Vector<SVGTextFragment>& fragments = queryData->textBox->textFragments();
 
         // Loop over all text fragments in this text box, firing a callback for each.
@@ -146,7 +165,7 @@ bool SVGTextQuery::mapStartEndPositionsIntoFragmentCoordinates(Data* queryData,
     if (startPosition >= endPosition)
         return false;
 
-    modifyStartEndPositionsRespectingLigatures(queryData, startPosition, endPosition);
+    modifyStartEndPositionsRespectingLigaturesCached(queryData, startPosition, endPosition);

[pdr., 2014/06/24 21:18:48]

This name was confusing until I read the implementation, since I wasn't sure
when one would call x() vs xCached(). Since this is the only caller, can you
move the cache logic into the top of modifyStartEndPositionsRespectingLigatures?

[fs, 2014/06/25 10:29:49]

Although it didn't like having the additional indirection, I felt that it
"packaged" the relevant bits in a good way (and allowed me to not worry about
multiple exit-points). Will fold as requested though.

     if (!queryData->textBox->mapStartEndPositionsIntoFragmentCoordinates(fragment, startPosition, endPosition))
         return false;
 
@@ -154,6 +173,23 @@ bool SVGTextQuery::mapStartEndPositionsIntoFragmentCoordinates(Data* queryData,
     return true;
 }
 
+void SVGTextQuery::modifyStartEndPositionsRespectingLigaturesCached(Data* queryData, int& startPosition, int& endPosition) const
+{
+    if (queryData->ligatureAdjustmentLastStartPositionQuery == startPosition && queryData->ligatureAdjustmentLastEndPositionQuery == endPosition) {
+        startPosition = queryData->ligatureAdjustmentLastStartPositionResult;
+        endPosition = queryData->ligatureAdjustmentLastEndPositionResult;
+        return;
+    }
+
+    queryData->ligatureAdjustmentLastStartPositionQuery = startPosition;
+    queryData->ligatureAdjustmentLastEndPositionQuery = endPosition;
+
+    modifyStartEndPositionsRespectingLigatures(queryData, startPosition, endPosition);
+
+    queryData->ligatureAdjustmentLastStartPositionResult = startPosition;
+    queryData->ligatureAdjustmentLastEndPositionResult = endPosition;
+}
+
 void SVGTextQuery::modifyStartEndPositionsRespectingLigatures(Data* queryData, int& startPosition, int& endPosition) const
 {
     SVGTextLayoutAttributes* layoutAttributes = queryData->textRenderer->layoutAttributes();