Optimize SVGTextQuery's character offset ligature adjustment query

Source/core/rendering/svg/SVGTextQuery.cpp

 /*
  * Copyright (C) Research In Motion Limited 2010-2012. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 20 matching lines @@
 
 namespace WebCore {
 
 // Base structure for callback user data
 struct SVGTextQuery::Data {
     Data()
         : isVerticalText(false)
         , 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)
 {
     if (!renderer)
         return 0;
 
     if (renderer->isRenderBlock()) {
         // If we're given a block element, it has to be a RenderSVGText.
         ASSERT(renderer->isSVGText());
@@ skipping 46 matching lines @@
 
 bool SVGTextQuery::executeQuery(Data* queryData, ProcessTextFragmentCallback fragmentCallback) const
 {
     ASSERT(!m_textBoxes.isEmpty());
 
     unsigned processedCharacters = 0;
     unsigned textBoxCount = m_textBoxes.size();
 
     // 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.
         unsigned fragmentCount = fragments.size();
         for (unsigned i = 0; i < fragmentCount; ++i) {
             const SVGTextFragment& fragment = fragments.at(i);
             if ((this->*fragmentCallback)(queryData, fragment))
                 return true;
 
             processedCharacters += fragment.length;
         }
 
         queryData->processedCharacters = processedCharacters;
     }
 
     return false;
 }
 
 bool SVGTextQuery::mapStartEndPositionsIntoFragmentCoordinates(Data* queryData, const SVGTextFragment& fragment, int& startPosition, int& endPosition) const
 {
     // Reuse the same logic used for text selection & painting, to map our query start/length into start/endPositions of the current text fragment.
     startPosition -= queryData->processedCharacters;
     endPosition -= queryData->processedCharacters;
 
     startPosition = max(0, startPosition);
 
     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;
 
     ASSERT(startPosition < endPosition);
     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();
     Vector<SVGTextMetrics>& textMetricsValues = layoutAttributes->textMetricsValues();
     unsigned boxStart = queryData->textBox->start();
     unsigned boxLength = queryData->textBox->len();
 
     unsigned textMetricsOffset = 0;
     unsigned textMetricsSize = textMetricsValues.size();
 
@@ skipping 374 matching lines @@
         return -1;
 
     CharacterNumberAtPositionData data(position);
     if (!executeQuery(&data, &SVGTextQuery::characterNumberAtPositionCallback))
         return -1;
 
     return data.processedCharacters;
 }
 
 }