Bug 65711 – Large SVG text layout performance regression in r81168

• Status: REOPENED
• Component: SVG

[Tim Horton]

Using the attached test document, one can reproduce a noticeable and significant performance regression in SVG text layout between r81167 and r81168 (the commit which fixed BiDi support in SVG text) which has not improved in ToT.

Samples from ToT (r92290) attached.

[Tim Horton]

Created attachment 102958 [details]
repro

[Tim Horton]

Created attachment 102959 [details]
sample on r92290

[Tim Horton]

<rdar://problem/9898636>

[Tim Horton]

Niko, do you have any idea if there's a relatively straightforward way to fix this, and/or do you plan on working on it any time soon? CC'ing a few more people to see if anyone has any ideas.

[Simon Fraser (smfr)]

Niko, what is your plan to address this serious perf regression?

[Tim Horton]

There's also a second (less significant, about 2x) regression in http://trac.webkit.org/changeset/82411 which I missed when manually testing.

[Nikolas Zimmermann]

(In reply to comment #5)
> Niko, what is your plan to address this serious perf regression?

I knew that we'd regress performance when this patch was landed. Correctness over performance was the initial goal. SVGs demand to reorder the x/y/dx/dy value lists requires a lot more work compared to HTML (figuring out the correct order of the new lists, after BiDi algorithm ran..)

Anyhow there's no need to perform the whole logic if the x/y/dx/dy/rotate lists are empty and/or consist of a single value (which is the common case!).

I'm sure I have an older patch lying around which attempted to fix that, I'll have to look it up.
But first I'm updating my patch for bug 47156, as I need to get that landed first, as it blocks any other work...

[Nikolas Zimmermann]

I've time profiled your testcase with Instruments. Jeez, heavy regression, confirmed.
I wouldn't want to believe it's that slow :-)

The problem is following:


void RenderSVGText::layout()
...

    if (m_needsPositioningValuesUpdate) {
        // Perform SVG text layout phase one (see SVGTextLayoutAttributesBuilder for details).
        SVGTextLayoutAttributesBuilder layoutAttributesBuilder;
        layoutAttributesBuilder.buildLayoutAttributesForTextSubtree(this);
....

Whenever the x/y/dx/dy/rotate lists change, we have to recompute all values for the whole subtree.
(This could be done more efficiently as well, but for now let's accept it).

The heavy text layout code in SVGTextLayoutAttributesBuilder should run:
a) when the <text> subtree is laid out the first time, in order to store text layout attributes in all RenderSVGInlineText boxes, needed for the inline box tree creation (text chunk boundaries need to be known in RenderBlockLineLayout, when deciding when to create inline text boxes)

b) if x/y/dx/dy/rotate of any <text> element or any of its kids change

That's the theory. So let's look if we're really doing that :-)
RenderSVGText::setNeedsPositioningValuesUpdate() is called to mark RenderSVGText to run the SVGTextLayoutAttributesBuilder during the next layout.

#1) SVGTextContentElement.cpp:        textRenderer->setNeedsPositioningValuesUpdate();
- called by SVGTextContentElement::childrenChanged, only for scripted (aka. non parser) changes

#2) SVGTextPositioningElement.cpp:            textRenderer->setNeedsPositioningValuesUpdate();
- called by SVGTextPositioningElement::svgAttributeChanged, whenever x/y/dx/dy/rotate change.

#3) RenderSVGInline.cpp:        textRenderer->setNeedsPositioningValuesUpdate();
- called by RenderSVGInline::willBeDestroyed()

#4) RenderSVGInlineText.cpp:        textRenderer->setNeedsPositioningValuesUpdate();
- called by RenderSVGInlineText::willBeDestroyed()

#5) RenderSVGInlineText.cpp:            textRenderer->setNeedsPositioningValuesUpdate();
- called by RenderSVGInlineText::styleDidChange() if styleDiff is NeedsLayout.

I think #1) is completely wrong, these changes should be handled in the renderers.
Number #2) is the place where x/y/dx/dy/rotate changes are captured, this is the only correct place.

Points #3) - #5) ruin are performance currently!

We're running the _whole_ first phase of the text layout for the whole tree whenever a single eg. <tspan> gets removed from it. You might wonder why at all?
<text x="10 20 30"><tspan>A</tspan>B<tspan>C</tspan></text>
A: x="10"
B: x="20"
C: y="30"

When the first <tspan> gets removed from DOM, then B will have to carry x="10" and C y="20".
... As you can see it's quite involved to correctly recalculate the x/y/dx/dy/rotate list information that will need to be kept in the individual renderers.

But this should ONLY need to happen if the x/y/dx/dy/rotate value lists actually consist of MORE than one value. The whole work we're doing is useless for your testcase.

I'll continue thinking about how to implement a fast-path for SVG text layout. Stay tuned...

Note: There are numerous of other places, shown by Instruments, that need optimization, but I think this is by far the worst problem for this testcase.

[Nikolas Zimmermann]

Since two days I own a shiny new 4-core iMac, with a SSD, lots of ram, and I can finally profile again :-)
The regression is gone locally: my local dbg build gets around 30 fps now, compared to Safari5 release, which is around 15 fps on my machine. trunk was below 1fps... (All checked using QuartzMeter).

It'll take some more days to finish it, but then we're fast again.

[Tim Horton]

(In reply to comment #9)
> Since two days I own a shiny new 4-core iMac, with a SSD, lots of ram, and I can finally profile again :-)
> The regression is gone locally: my local dbg build gets around 30 fps now, compared to Safari5 release, which is around 15 fps on my machine. trunk was below 1fps... (All checked using QuartzMeter).
> 
> It'll take some more days to finish it, but then we're fast again.

Music to my ears! I can't wait :-)

[Nikolas Zimmermann]

Created attachment 120890 [details]
Full patch prototype

Uploading an early version to receive EWS results. I see about 58 FPS now! Opera is only at about 15 FPS.

[Nikolas Zimmermann]

(In reply to comment #11)
> Uploading an early version to receive EWS results. I see about 58 FPS now! Opera is only at about 15 FPS.

I will start breaking up this patch in smaller pieces, just in the way I initially prototyped this.
Some more numbers:

Without patch:
trunk release build, ~ 5 FPS, memory consumption (RPRVT) 35 MB
patch release build ~ 58 FPS,  memory consumption (RPRVT) 9076K

w/o patch:
PID       COMMAND             %CPU     TIME        #TH     #WQ     #PORTS  #MREGS   RPRVT     RSHRD     RSIZE     VPRVT     VSIZE     PGRP     PPID     STATE       UID     FAULTS      COW       MSGSENT        MSGRECV        SYSBSD       SYSMACH      CSW          PAGEINS     KPRVT     KSHRD     USER
47503     WebProcess          0.0      00:00.15    7       1       105     158      9076K     28M       28M       351M      3566M     47492    47502    sleeping    501     10597       808       3736           1818           3184+        3342         679+         1           484K      232K      nzimmermann

with patch:
50700     WebProcess          0.0      00:02.80    8       2       125     290      35M       8692K     56M       221M      3559M     98010    98010    sleeping    501     341911      419       51586          25339          52765+       58377        22414+       3           700K      2357K     nzimmermann

There are no regressions, and its way faster than it ever was in WebKit.
Instruments still shows lots of low hanging fruits, I bet we can easily double our performance again, with some clever tricks regarding the line box construction.

As I didn't include a ChangeLog in the patch, here's a short summary of what its about:
- x/y/dx/dy/rotate lists were fixed size lists each as long as the whole text for each RenderSVGInlineText, mostly padded with empty values in the common case (if no absolute positions were specified) - wasting tons of memory. That alone account for half the men reduction.

- Any DOM mutation, adding a node or removing a node, changing a character data node, caused the whole <text> subtree (consisting of 100+ tspans, in the attached test case) to be remeasured. Each character was measured not only once, but we always measured ranges: i .. i +1, and 0 .. i, to account for eg. Arabic text where the length of a chunk of text is not equal to the sum of the widths.
To fix that properly, I had to separate the Vector<SVGTextMetrics> from the SVGTextLayoutAttributes (which are stored across the render tree in RenderSVGInlineText members) data structure, and cache it fully separated in RenderSVGText. Why is that?
I'm now building a HashMap<SVGTextLayoutAttributes*, OwnPtr<Vector<SVGTextMetrics> > > in RenderSVGText, that associates SVGTextLayoutAttributes (which in turn belong to one single RenderSVGInlineText object) with a vector of metrics values per character.

Any mutations of the render tree have to be tracked:
- if a RenderSVGInlineText disappears (willBeDestroyed) we have to lookup its SVGTextLayoutAttributes in the "Metrics Map" (HashMap<> from above), and remove it from the hash map.
- if a RenderSVGInlineText is added to the tree (addChild), we have to measure its metrics values, and 

I didn't do this when I initially wrote the SVGTextLayoutEngine, as it has side-effects. The removal of a RenderSVGInlineText object, can affect layout attributes of its previous and next sibling!

Consider: <text x="10" y="10">A<tspan> </tspan> B</tspan>
This would render as "A_B" where _ is a space coming from the <tspan>. The additional leading space in the " B" run is ignored, as xml:space is not set to "preserve". Here's a DRT dump for it:

        RenderSVGText {text} at (10,-4) size 27x18 contains 1 chunk(s)
          RenderSVGInlineText {#text} at (0,0) size 12x18
            chunk 1 text run 1 at (10.00,10.00) startOffset 0 endOffset 1 width 12.00: "A"
          RenderSVGTSpan {tspan} at (0,0) size 4x18
            RenderSVGInlineText {#text} at (12,0) size 4x18
              chunk 1 text run 1 at (22.00,10.00) startOffset 0 endOffset 1 width 4.00: " "
          RenderSVGInlineText {#text} at (16,0) size 11x18
            chunk 1 text run 1 at (26.00,10.00) startOffset 0 endOffset 1 width 11.00: "B"

If we remove the <tspan> #text node child, what happens?

        RenderSVGText {text} at (10,-4) size 27x18 contains 1 chunk(s)
          RenderSVGInlineText {#text} at (0,0) size 12x18
            chunk 1 text run 1 at (10.00,10.00) startOffset 0 endOffset 1 width 12.00: "A"
          RenderSVGTSpan {tspan} at (0,0) size 0x0
          RenderSVGInlineText {#text} at (12,0) size 15x18
            chunk 1 text run 1 at (22.00,10.00) startOffset 0 endOffset 2 width 15.00: " B"

The leading space of the " B" now becomes significant, and is no longer ignored, as there is previous character is no longer a space, but "A".

Each time a renderer is removed from the tree, we have to lookup the SVGTextLayoutAttributes* associated with the "logical" previous & next renderers, and remeasure them as well, not just the newly added RenderSVGInlineText.

To make a long story short, we now have a SVGMetricsMap thats associated with the RenderSVGText, and not in SVGRootInlineBox or somewhere in the line box tree, which doesn't survive layouts() - its rebuilt every time RenderSVGText is laid out. Any render tree mutations are reflected in the SVGMetricsMap before SVGRootInlineBox::computePerCharacterLayoutInformation() is called (aka. before the line box tree is constructed). SVGTextLayoutEngine is driven by the SVGRootInlineBox, at this point its always guaranteed that the SVGMetricsMap contains accurate metrics for the whole subtree, which is needed to figure out the glyph advance when executing the SVG Text layout algorithm.

Numerous little fixes and optimizations combined yield a huge speed up, and there's still lots of room for improvement :-)

[Nikolas Zimmermann]

(In reply to comment #12)
Oops, this is reversed. With patch:

> PID       COMMAND             %CPU     TIME        #TH     #WQ     #PORTS  #MREGS   RPRVT     RSHRD     RSIZE     VPRVT     VSIZE     PGRP     PPID     STATE       UID     FAULTS      COW       MSGSENT        MSGRECV        SYSBSD       SYSMACH      CSW          PAGEINS     KPRVT     KSHRD     USER
> 47503     WebProcess          0.0      00:00.15    7       1       105     158      9076K     28M       28M       351M      3566M     47492    47502    sleeping    501     10597       808       3736           1818           3184+        3342         679+         1           484K      232K      nzimmermann
> 
> with patch:
Wrong, without patch:

> 50700     WebProcess          0.0      00:02.80    8       2       125     290      35M       8692K     56M       221M      3559M     98010    98010    sleeping    501     341911      419       51586          25339          52765+       58377        22414+       3           700K      2357K     nzimmermann

Numbers are still all true.

[Nikolas Zimmermann]

Created attachment 120942 [details]
Patch chunk 1, v1

[Gyuyoung Kim]

Comment on attachment 120942 [details]
Patch chunk 1, v1

Attachment 120942 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11003289

[WebKit Review Bot]

Comment on attachment 120942 [details]
Patch chunk 1, v1

Attachment 120942 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11074277

[Early Warning System Bot]

Comment on attachment 120942 [details]
Patch chunk 1, v1

Attachment 120942 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11043305

[Nikolas Zimmermann]

Created attachment 120945 [details]
Patch chunk 1, v2

[Gustavo Noronha (kov)]

Comment on attachment 120945 [details]
Patch chunk 1, v2

Attachment 120945 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11081002

[Gyuyoung Kim]

Comment on attachment 120945 [details]
Patch chunk 1, v2

Attachment 120945 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10954478

[Nikolas Zimmermann]

Created attachment 120947 [details]
Patch chunk 1, v3

[WebKit Review Bot]

Comment on attachment 120947 [details]
Patch chunk 1, v3

Attachment 120947 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11083010

[Early Warning System Bot]

Comment on attachment 120947 [details]
Patch chunk 1, v3

Attachment 120947 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11001609

[Nikolas Zimmermann]

Created attachment 121069 [details]
Patch chunk 1, v4

[Early Warning System Bot]

Comment on attachment 121069 [details]
Patch chunk 1, v4

Attachment 121069 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11084475

[Dirk Schulze]

Comment on attachment 121069 [details]
Patch chunk 1, v4

View in context: https://bugs.webkit.org/attachment.cgi?id=121069&action=review

I continue review this evening. Stopping at SVGTextLayoutAttributes for now.

> Source/WebCore/rendering/svg/RenderSVGInlineText.cpp:150
> +    const SVGCharacterDataMap::const_iterator it = m_attributes->characterDataMap().find(static_cast<unsigned>(position));

Can't position be unsigned on the function call? If not explain it please and add an ASSERT.

> Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:93
> +    // FIXME: This information will be cached in RenderSVGText, in a follow-up patch.

bug number please

> Source/WebCore/rendering/svg/SVGTextLayoutAttributes.h:52
> +struct SVGCharacterDataMap : HashMap<unsigned, SVGCharacterData, DefaultHash<unsigned>::Hash, SVGCharacterDataMapHashTraits> {

How do you use the unsigned? Do you really need to create a new class/type or that? Isn"t it possible to move every entry one step further?

[Dirk Schulze]

Comment on attachment 121069 [details]
Patch chunk 1, v4

View in context: https://bugs.webkit.org/attachment.cgi?id=121069&action=review

As far as I can tell the code changes look reasonable. But I lost the overview. Isn't it possible that you make smaller steps? Like just the step of SVGTextLayoutAttributes to SVGTextLayoutAttributes* and so on? These might not be a performance improvements, but it wouldn't worry :). Moving of code would be a second step and so on.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:24999
> +				59DE790613F16C8C0007FCDF /* JavaMethodJSC.h in Headers */,
> +				BCBB8AB813F1AFB000734DF0 /* PODArena.h in Headers */,
> +				BCBB8AB913F1AFB000734DF0 /* PODInterval.h in Headers */,
> +				BCBB8ABA13F1AFB000734DF0 /* PODIntervalTree.h in Headers */,
> +				BCBB8ABB13F1AFB000734DF0 /* PODRedBlackTree.h in Headers */,
> +				FD6F252D13F5EF0E0065165F /* MediaElementAudioSourceNode.h in Headers */,
> +				FD23A12613F5FA5900F67001 /* JSMediaElementAudioSourceNode.h in Headers */,
> +				4998AEC613F9D0EA0090B1AA /* RequestAnimationFrameCallback.h in Headers */,
> +				4998AECE13F9D6C90090B1AA /* JSRequestAnimationFrameCallback.h in Headers */,
> +				4998AED213FB224D0090B1AA /* ScriptedAnimationController.h in Headers */,
> +				DF9AFD7213FC31D80015FEB7 /* MediaPlayerPrivateAVFoundationObjC.h in Headers */,
> +				93500F3213FDE3BE0099EC24 /* NSScrollerImpDetails.h in Headers */,
> +				D0A3A7311405A39800FB8ED3 /* ResourceLoaderOptions.h in Headers */,
> +				BCE4389C140B1BA8005E437E /* JSDictionary.h in Headers */,
> +				E45322AC140CE267005A0F92 /* SelectorQuery.h in Headers */,
> +				B10B6980140C174000BC1C26 /* WebVTTToken.h in Headers */,
> +				B10B6982140C174000BC1C26 /* WebVTTTokenizer.h in Headers */,
> +				9B0FB192140DB5790022588F /* HTTPValidation.h in Headers */,
> +				BC2CBF4E140F1ABD003879BE /* JSWebGLContextEvent.h in Headers */,
> +				BC274B2F140EBEB200EADFA6 /* CSSBorderImageSliceValue.h in Headers */,
> +				E44B4BB4141650D7002B1D8B /* SelectorChecker.h in Headers */,
> +				978AD67514130A8D00C7CAE3 /* HTMLSpanElement.h in Headers */,
> +				9752D38E1413104B003305BD /* JSHTMLSpanElement.h in Headers */,
> +				1A927FD21416A15B003A83C8 /* npapi.h in Headers */,
> +				1A927FD31416A15B003A83C8 /* npruntime.h in Headers */,
> +				1A927FD41416A15B003A83C8 /* nptypes.h in Headers */,

Why does your Xcode add all these headers?

> Source/WebCore/rendering/svg/RenderSVGText.cpp:43
>  #include "SVGResourcesCache.h"
>  #include "SVGRootInlineBox.h"

Needs copyright (CR) notice.

> Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:72
>  void SVGRootInlineBox::paint(PaintInfo& paintInfo, const LayoutPoint&, LayoutUni
>      SVGRenderSupport::finishRenderSVGContent(boxRenderer, childPaintInfo, paintInfo.context);
>  }

CR notice

> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:365
> +           

Unnecessary spaces.

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:55
> +    const UChar* characters = text->characters();
> +    unsigned textLength = text->textLength();
> +    bool preserveWhiteSpace = text->style()->whiteSpace() == PRE;
> +
> +    float lastStartToCurrentWidth = 0;
> +    SVGTextMetrics startToCurrentMetrics;
> +    SVGTextMetrics currentMetrics;
> +
> +    unsigned metricsLength = 1;
> +    for (unsigned textPosition = 0; textPosition < textLength; textPosition += metricsLength) {
> +        const UChar* currentCharacter = characters + textPosition;
> +        bool currentCharacterIsSpace = false;
> +        if (U16_IS_LEAD(*currentCharacter) && (textPosition + 1) < textLength && U16_IS_TRAIL(characters[textPosition + 1])) {
> +            // Handle surrogate pairs.
> +            startToCurrentMetrics = SVGTextMetrics::measureCharacterRange(text, 0, textPosition + 2);
> +            currentMetrics = SVGTextMetrics::measureCharacterRange(text, textPosition, 2);
> +            metricsLength = currentMetrics.length();
> +        } else {

It partly looks similar to skipTextRenderer. Note that I did not compared both functions, but can we share code here?

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:99
> +    const UChar* characters = text->characters();
> +    unsigned textLength = text->textLength();
> +    bool preserveWhiteSpace = text->style()->whiteSpace() == PRE;
> +    WidthIterator it(&scaledFont, run);
> +
> +    float totalWidth = 0;
> +    unsigned metricsLength = 1;
> +    for (unsigned textPosition = 0; textPosition < textLength; textPosition += metricsLength) {
> +        const UChar* currentCharacter = characters + textPosition;
> +        float currentWidth = 0;
> +        bool currentCharacterIsSpace = false;
> +        if (U16_IS_LEAD(*currentCharacter) && (textPosition + 1) < textLength && U16_IS_TRAIL(characters[textPosition + 1])) {
> +            // Handle surrogate pairs.

Looks also similar. Is it possible to abstract the functionality more and reuse this code?

[Nikolas Zimmermann]

Created attachment 121813 [details]
Patch chunk 1, v5

[Nikolas Zimmermann]

Comment on attachment 121813 [details]
Patch chunk 1, v5

Thanks Dirk for the first round of review - I realized its still way too complex, so I am starting with a small chunk this time: only the introduction of SVGTextMetricsBuilder, its not yet used.

[Early Warning System Bot]

Comment on attachment 121813 [details]
Patch chunk 1, v5

Attachment 121813 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11166938

[Nikolas Zimmermann]

Created attachment 121819 [details]
Patch chunk 1, v6

[Zoltan Herczeg]

Comment on attachment 121813 [details]
Patch chunk 1, v5

I think this patch is good in overall, only needs some polishing.

View in context: https://bugs.webkit.org/attachment.cgi?id=121813&action=review

> Source/WebCore/ChangeLog:31
> +        (WebCore::SVGTextMetrics::isEmpty): Add new isEmpty method, which replaces the == empyMetrics() invocations.

the == empTyMetrics()

> Source/WebCore/platform/graphics/Font.h:228
> +    bool textRunNeedsComplexCodePath(const TextRun& run) const { return codePath(run) == Complex; }

Put it after "CodePath codePath(const TextRun&) const;"
Btw, do you think this function increases understandability? To me "codePath(run) == Complex" seems quite clear...

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:34
> +SVGTextMetricsBuilder::SVGTextMetricsBuilder()
> +{
> +}

Could be an inlined constructor.

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:106
> +            currentCharacterIsSpace = *currentCharacter == ' ';

There are several space like characters in Unicode. Do you only need to check this one?

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:131
> +static void measureAllCharacters(RenderSVGInlineText* text, unsigned&, const UChar*& lastCharacter, void* userData)
> +{
> +    Vector<SVGTextMetrics>& textMetricsValues = *reinterpret_cast<Vector<SVGTextMetrics>*>(userData);

void* argument???

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:165
> +
> +    if (0) {
> +        unsigned dummy = 0;
> +        const UChar* lastCharacter = 0;
> +        measureAllCharacters(text, dummy, lastCharacter, 0);
> +    }
> +
> +    // FIXME: Enable this in a follow-up patch.
> +    // textRoot->walkSVGTextTree(text, measureAllCharacters, &textMetricsValues);

This is ugly. Couldn't we add the prototype to textRoot and put an ASSERT_NOT_REACHED there?

[Nikolas Zimmermann]

(In reply to comment #32)
> (From update of attachment 121813 [details])
> I think this patch is good in overall, only needs some polishing.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=121813&action=review
> the == empTyMetrics()
Fixed.

> > Source/WebCore/platform/graphics/Font.h:228
> > +    bool textRunNeedsComplexCodePath(const TextRun& run) const { return codePath(run) == Complex; }
> 
> Put it after "CodePath codePath(const TextRun&) const;"
I did not do this as codePath() is private.
> Btw, do you think this function increases understandability? To me "codePath(run) == Complex" seems quite clear...
Right, and I only did this as the CodePath is intended to be private.

> > Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:34
> Could be an inlined constructor.
Will fix.

> 
> > Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:106
> > +            currentCharacterIsSpace = *currentCharacter == ' ';
> 
> There are several space like characters in Unicode. Do you only need to check this one?
Yes, the line layout code in RenderBlockLineLayout.cpp, see shouldSkipWhitespaceAfterStartObject(), only cares about ASCII spaces when deciding where the start/end locations of an InlineBox are.

> 
> > Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:131
> > +static void measureAllCharacters(RenderSVGInlineText* text, unsigned&, const UChar*& lastCharacter, void* userData)
> > +{
> > +    Vector<SVGTextMetrics>& textMetricsValues = *reinterpret_cast<Vector<SVGTextMetrics>*>(userData);
> 
> void* argument???
Yes, walkSVGTextTree gets a callback function, see the original prototype patch. The walksVGTextTree() method is not added yet in this patch.
 
> > Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:165
> This is ugly. Couldn't we add the prototype to textRoot and put an ASSERT_NOT_REACHED there?
Ok, that's better, will fix.

[Nikolas Zimmermann]

Created attachment 121846 [details]
Patch chunk 1, v7

[Gyuyoung Kim]

Comment on attachment 121846 [details]
Patch chunk 1, v7

Attachment 121846 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11196072

[Nikolas Zimmermann]

Created attachment 121849 [details]
Patch chunk 1, v8

[Early Warning System Bot]

Comment on attachment 121849 [details]
Patch chunk 1, v8

Attachment 121849 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11198082

[Nikolas Zimmermann]

Created attachment 121876 [details]
Patch chunk 1, v9

[Zoltan Herczeg]

Comment on attachment 121876 [details]
Patch chunk 1, v9

r=me

View in context: https://bugs.webkit.org/attachment.cgi?id=121876&action=review

Just one note:

> Source/WebCore/platform/graphics/Font.h:229
> +    bool textRunNeedsComplexCodePath(const TextRun& run) const { return codePath(run) == Complex; }
> +

I think codePath should be made public. The enum is already public, and this makes the function "half" public.

[Nikolas Zimmermann]

Committed r104683: <http://trac.webkit.org/changeset/104683>

[Nikolas Zimmermann]

Reopening, webkit-patch land was too quick :-)

[Nikolas Zimmermann]

Created attachment 122197 [details]
Patch chunk 2, v1

[Gustavo Noronha (kov)]

Comment on attachment 122197 [details]
Patch chunk 2, v1

Attachment 122197 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11213373

[WebKit Review Bot]

Comment on attachment 122197 [details]
Patch chunk 2, v1

Attachment 122197 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11213374

[Early Warning System Bot]

Comment on attachment 122197 [details]
Patch chunk 2, v1

Attachment 122197 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11118353

[Gyuyoung Kim]

Comment on attachment 122197 [details]
Patch chunk 2, v1

Attachment 122197 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11211359

[Nikolas Zimmermann]

Created attachment 122202 [details]
Patch chunk 2, v2

[Antti Koivisto]

Comment on attachment 122202 [details]
Patch chunk 2, v2

View in context: https://bugs.webkit.org/attachment.cgi?id=122202&action=review

r=me

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Finish SVGTextMetricsBuilder introduction, tested in my local svg-text-performance branch.
> +        Refactor code, and polish it.
> +
> +        Doesn't affect any tests yet.

You should explain what you are changing in this patch in some more detail.

[Nikolas Zimmermann]

Created attachment 122411 [details]
Patch chunk 2, v3

[Antti Koivisto]

Comment on attachment 122411 [details]
Patch chunk 2, v3

View in context: https://bugs.webkit.org/attachment.cgi?id=122411&action=review

> Source/WebCore/ChangeLog:26
> +           stopAtLeaf!=0:
> +           This will only be used if we

some random looking text fragments here

> Source/WebCore/ChangeLog:47
> +        (WebCore::SVGTextMetricsBuilder::measureAllCharactersInLayoutAttributes):

I think you renamed this.

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.h:41
> +    void measureAllCharactersOfRenderer(RenderSVGInlineText*);

The naming seem slightly inconsistent...

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.h:52
> +    void initializeMeasurementWithTextRenderer(RenderSVGInlineText*);
> +    void walkTree(RenderObject*, RenderSVGInlineText* stopAtLeaf, MeasureTextData*);
> +    void measureTextRenderer(RenderSVGInlineText*, MeasureTextData*);

...with these (*Renderer vs. *TextRenderer). The argument type is the same.

[Nikolas Zimmermann]

(In reply to comment #50)
Fixed all issues, renamed measureAllCharactersOfRenderer to measureTextRenderer, taking a RenderSVGInlineText* only.

[Nikolas Zimmermann]

Committed r104926: <http://trac.webkit.org/changeset/104926>

[Nikolas Zimmermann]

Created attachment 122605 [details]
Patch chunk 3, v1

[WebKit Review Bot]

Comment on attachment 122605 [details]
Patch chunk 3, v1

Attachment 122605 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11256029

[Gyuyoung Kim]

Comment on attachment 122605 [details]
Patch chunk 3, v1

Attachment 122605 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11135116

[Nikolas Zimmermann]

Created attachment 122606 [details]
Patch chunk 3, v2

[Gyuyoung Kim]

Comment on attachment 122606 [details]
Patch chunk 3, v2

Attachment 122606 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11255114

[WebKit Review Bot]

Comment on attachment 122606 [details]
Patch chunk 3, v2

Attachment 122606 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11214111

[Nikolas Zimmermann]

Created attachment 122608 [details]
Patch chunk 3, v3

[Zoltan Herczeg]

Comment on attachment 122608 [details]
Patch chunk 3, v3

r=me

Carefully written patch.

[Nikolas Zimmermann]

Committed r105057: <http://trac.webkit.org/changeset/105057>

[Nikolas Zimmermann]

Created attachment 122622 [details]
Patch chunk 4, v1

[Zoltan Herczeg]

Comment on attachment 122622 [details]
Patch chunk 4, v1

Images looks the same. rs=me

[Nikolas Zimmermann]

Committed r105061: <http://trac.webkit.org/changeset/105061>

[Nikolas Zimmermann]

Created attachment 122649 [details]
Final patch chunk, v1

[WebKit Review Bot]

Comment on attachment 122649 [details]
Final patch chunk, v1

Attachment 122649 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11111103

New failing tests:
svg/carto.net/window.svg
svg/text/remove-tspan-from-text.html
svg/custom/use-detach.svg
svg/custom/js-late-clipPath-and-object-creation.svg
svg/custom/js-late-gradient-and-object-creation.svg
svg/text/modify-text-node-in-tspan.html
svg/text/remove-text-node-from-tspan.html
svg/custom/js-late-pattern-and-object-creation.svg
svg/text/append-text-node-to-tspan.html

[Zoltan Herczeg]

Comment on attachment 122649 [details]
Final patch chunk, v1

View in context: https://bugs.webkit.org/attachment.cgi?id=122649&action=review

> Source/WebCore/rendering/svg/RenderSVGInlineText.h:38
> -    SVGTextLayoutAttributes& layoutAttributes() { return m_layoutAttributes; }
> +    SVGTextLayoutAttributes* layoutAttributes() { return &m_layoutAttributes; }

What is the reason to change this to pointer?

> Source/WebCore/rendering/svg/RenderSVGText.h:95
> -    Vector<SVGTextLayoutAttributes> m_layoutAttributes;
> +    Vector<SVGTextLayoutAttributes*> m_layoutAttributes;

From this point the vector does not own the attributes anymore, how do you free them?

[Nikolas Zimmermann]

(In reply to comment #67)
> (From update of attachment 122649 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122649&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGInlineText.h:38
> > -    SVGTextLayoutAttributes& layoutAttributes() { return m_layoutAttributes; }
> > +    SVGTextLayoutAttributes* layoutAttributes() { return &m_layoutAttributes; }
> 
> What is the reason to change this to pointer?
Simplicity, all call sites use SVGTextLayoutAttributes by pointer.
 
> > Source/WebCore/rendering/svg/RenderSVGText.h:95
> > -    Vector<SVGTextLayoutAttributes> m_layoutAttributes;
> > +    Vector<SVGTextLayoutAttributes*> m_layoutAttributes;
> 
> From this point the vector does not own the attributes anymore, how do you free them?
I don't need to free them, this Vector only ever contains pointers to SVGTextLayoutAttributes stored in the RenderSVGInlineText objects. If a RenderSVGInlineText disappears, its SVGTextLayoutAttributes pointer is removed from the Vector. It's kept in sync.

[Zoltan Herczeg]

Comment on attachment 122649 [details]
Final patch chunk, v1

r=me
Probably some layout tests will fail so please check the bots after landing.

[Nikolas Zimmermann]

Committed r105143: <http://trac.webkit.org/changeset/105143>

[Nikolas Zimmermann]

(In reply to comment #69)
> (From update of attachment 122649 [details])
> r=me
> Probably some layout tests will fail so please check the bots after landing.

Will do. Landed new gtk results in r105144.

[Nikolas Zimmermann]

Landed Qt results in r105145. Win bots early exit, so I'm unable to grab win results. Chromium expectations were updated, and it seems to work - closing bug.

[Csaba Osztrogonác]

It caused an assertion on Qt on svg/carto.net/frameless-svg-parse-error.html:

ASSERTION FAILED: fontCache()->generation() == m_generation
../../../../Source/WebCore/platform/graphics/FontFallbackList.cpp(104) : const WebCore::FontData* WebCore::FontFallbackList::fontDataAt(const WebCore::Font*, unsigned int) const

[Nikolas Zimmermann]

(In reply to comment #73)
> It caused an assertion on Qt on svg/carto.net/frameless-svg-parse-error.html:
> 
> ASSERTION FAILED: fontCache()->generation() == m_generation
> ../../../../Source/WebCore/platform/graphics/FontFallbackList.cpp(104) : const WebCore::FontData* WebCore::FontFallbackList::fontDataAt(const WebCore::Font*, unsigned int) const

Zoltan had a fix for this, if I recall correctly? Do we still have this problem?

[Eric Seidel (no email)]

Comment on attachment 121876 [details]
Patch chunk 1, v9

Cleared Zoltan Herczeg's review+ from obsolete attachment 121876 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 122411 [details]
Patch chunk 2, v3

Cleared Antti Koivisto's review+ from obsolete attachment 122411 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 122608 [details]
Patch chunk 3, v3

Cleared Zoltan Herczeg's review+ from obsolete attachment 122608 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 122649 [details]
Final patch chunk, v1

Cleared Zoltan Herczeg's review+ from obsolete attachment 122649 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Ahmad Saleem]

This Blink commit suggests that it landed when Blink / WebKit were same. https://src.chromium.org/viewvc/blink?view=revision&revision=105143

Do we need to keep it open?