Bug 65711 - Large SVG text layout performance regression in r81168
Summary: Large SVG text layout performance regression in r81168
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Nikolas Zimmermann
URL: http://trac.webkit.org/changeset/81168
Keywords: InRadar
Depends on: 76534
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-04 12:42 PDT by Tim Horton
Modified: 2023-05-27 16:01 PDT (History)
15 users (show)

See Also:


Attachments
repro (11.79 KB, application/zip)
2011-08-04 12:43 PDT, Tim Horton
no flags Details
sample on r92290 (488.97 KB, text/plain)
2011-08-04 12:44 PDT, Tim Horton
no flags Details
Full patch prototype (168.07 KB, patch)
2012-01-02 12:15 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 1, v1 (1.80 MB, patch)
2012-01-03 07:05 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 1, v2 (1.13 MB, patch)
2012-01-03 07:46 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 1, v3 (1.13 MB, patch)
2012-01-03 08:09 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 1, v4 (1.14 MB, patch)
2012-01-03 22:43 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 1, v5 (30.44 KB, patch)
2012-01-10 02:13 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 1, v6 (30.40 KB, patch)
2012-01-10 03:10 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 1, v7 (33.95 KB, patch)
2012-01-10 07:39 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 1, v8 (33.26 KB, patch)
2012-01-10 08:05 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 1, v9 (33.45 KB, patch)
2012-01-10 11:27 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 2, v1 (15.78 KB, patch)
2012-01-12 02:36 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 2, v2 (15.78 KB, patch)
2012-01-12 03:17 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 2, v3 (17.01 KB, patch)
2012-01-13 04:43 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 3, v1 (60.00 KB, patch)
2012-01-16 02:00 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 3, v2 (60.01 KB, patch)
2012-01-16 02:30 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 3, v3 (60.62 KB, patch)
2012-01-16 03:16 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch chunk 4, v1 (1.04 MB, patch)
2012-01-16 05:50 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch chunk, v1 (713.95 KB, patch)
2012-01-16 09:03 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2011-08-04 12:42:46 PDT
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.
Comment 1 Tim Horton 2011-08-04 12:43:35 PDT
Created attachment 102958 [details]
repro
Comment 2 Tim Horton 2011-08-04 12:44:36 PDT
Created attachment 102959 [details]
sample on r92290
Comment 3 Tim Horton 2011-08-04 12:53:29 PDT
<rdar://problem/9898636>
Comment 4 Tim Horton 2011-09-22 14:36:23 PDT
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.
Comment 5 Simon Fraser (smfr) 2011-10-25 14:38:04 PDT
Niko, what is your plan to address this serious perf regression?
Comment 6 Tim Horton 2011-10-25 14:57:55 PDT
There's also a second (less significant, about 2x) regression in http://trac.webkit.org/changeset/82411 which I missed when manually testing.
Comment 7 Nikolas Zimmermann 2011-10-26 00:34:38 PDT
(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...
Comment 8 Nikolas Zimmermann 2011-10-28 00:13:21 PDT
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.
Comment 9 Nikolas Zimmermann 2011-12-21 08:04:10 PST
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.
Comment 10 Tim Horton 2011-12-21 08:06:55 PST
(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 :-)
Comment 11 Nikolas Zimmermann 2012-01-02 12:15:14 PST
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.
Comment 12 Nikolas Zimmermann 2012-01-03 01:36:02 PST
(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 :-)
Comment 13 Nikolas Zimmermann 2012-01-03 03:09:22 PST
(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.
Comment 14 Nikolas Zimmermann 2012-01-03 07:05:25 PST
Created attachment 120942 [details]
Patch chunk 1, v1
Comment 15 Gyuyoung Kim 2012-01-03 07:20:33 PST
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
Comment 16 WebKit Review Bot 2012-01-03 07:24:13 PST
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
Comment 17 Early Warning System Bot 2012-01-03 07:30:33 PST
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
Comment 18 Nikolas Zimmermann 2012-01-03 07:46:45 PST
Created attachment 120945 [details]
Patch chunk 1, v2
Comment 19 Gustavo Noronha (kov) 2012-01-03 07:57:19 PST
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
Comment 20 Gyuyoung Kim 2012-01-03 08:01:09 PST
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
Comment 21 Nikolas Zimmermann 2012-01-03 08:09:50 PST
Created attachment 120947 [details]
Patch chunk 1, v3
Comment 22 WebKit Review Bot 2012-01-03 08:31:04 PST
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
Comment 23 Early Warning System Bot 2012-01-03 08:38:53 PST
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
Comment 24 Nikolas Zimmermann 2012-01-03 22:43:49 PST
Created attachment 121069 [details]
Patch chunk 1, v4
Comment 25 Early Warning System Bot 2012-01-04 06:16:11 PST
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
Comment 26 Dirk Schulze 2012-01-04 16:26:55 PST
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?
Comment 27 Dirk Schulze 2012-01-08 22:14:52 PST
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?
Comment 28 Nikolas Zimmermann 2012-01-10 02:13:13 PST
Created attachment 121813 [details]
Patch chunk 1, v5
Comment 29 Nikolas Zimmermann 2012-01-10 02:17:23 PST
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.
Comment 30 Early Warning System Bot 2012-01-10 02:44:55 PST
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
Comment 31 Nikolas Zimmermann 2012-01-10 03:10:35 PST
Created attachment 121819 [details]
Patch chunk 1, v6
Comment 32 Zoltan Herczeg 2012-01-10 03:55:22 PST
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?
Comment 33 Nikolas Zimmermann 2012-01-10 05:02:33 PST
(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.
Comment 34 Nikolas Zimmermann 2012-01-10 07:39:29 PST
Created attachment 121846 [details]
Patch chunk 1, v7
Comment 35 Gyuyoung Kim 2012-01-10 07:52:28 PST
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
Comment 36 Nikolas Zimmermann 2012-01-10 08:05:03 PST
Created attachment 121849 [details]
Patch chunk 1, v8
Comment 37 Early Warning System Bot 2012-01-10 08:25:11 PST
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
Comment 38 Nikolas Zimmermann 2012-01-10 11:27:12 PST
Created attachment 121876 [details]
Patch chunk 1, v9
Comment 39 Zoltan Herczeg 2012-01-11 01:01:25 PST
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.
Comment 40 Nikolas Zimmermann 2012-01-11 01:16:42 PST
Committed r104683: <http://trac.webkit.org/changeset/104683>
Comment 41 Nikolas Zimmermann 2012-01-11 01:22:00 PST
Reopening, webkit-patch land was too quick :-)
Comment 42 Nikolas Zimmermann 2012-01-12 02:36:00 PST
Created attachment 122197 [details]
Patch chunk 2, v1
Comment 43 Gustavo Noronha (kov) 2012-01-12 02:50:15 PST
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
Comment 44 WebKit Review Bot 2012-01-12 02:53:18 PST
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
Comment 45 Early Warning System Bot 2012-01-12 02:56:22 PST
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
Comment 46 Gyuyoung Kim 2012-01-12 03:07:30 PST
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
Comment 47 Nikolas Zimmermann 2012-01-12 03:17:40 PST
Created attachment 122202 [details]
Patch chunk 2, v2
Comment 48 Antti Koivisto 2012-01-13 02:42:38 PST
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.
Comment 49 Nikolas Zimmermann 2012-01-13 04:43:15 PST
Created attachment 122411 [details]
Patch chunk 2, v3
Comment 50 Antti Koivisto 2012-01-13 05:41:19 PST
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.
Comment 51 Nikolas Zimmermann 2012-01-13 05:47:11 PST
(In reply to comment #50)
Fixed all issues, renamed measureAllCharactersOfRenderer to measureTextRenderer, taking a RenderSVGInlineText* only.
Comment 52 Nikolas Zimmermann 2012-01-13 06:18:49 PST
Committed r104926: <http://trac.webkit.org/changeset/104926>
Comment 53 Nikolas Zimmermann 2012-01-16 02:00:22 PST
Created attachment 122605 [details]
Patch chunk 3, v1
Comment 54 WebKit Review Bot 2012-01-16 02:05:46 PST
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
Comment 55 Gyuyoung Kim 2012-01-16 02:06:01 PST
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
Comment 56 Nikolas Zimmermann 2012-01-16 02:30:14 PST
Created attachment 122606 [details]
Patch chunk 3, v2
Comment 57 Gyuyoung Kim 2012-01-16 02:53:45 PST
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
Comment 58 WebKit Review Bot 2012-01-16 02:54:40 PST
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
Comment 59 Nikolas Zimmermann 2012-01-16 03:16:41 PST
Created attachment 122608 [details]
Patch chunk 3, v3
Comment 60 Zoltan Herczeg 2012-01-16 04:24:20 PST
Comment on attachment 122608 [details]
Patch chunk 3, v3

r=me

Carefully written patch.
Comment 61 Nikolas Zimmermann 2012-01-16 05:08:51 PST
Committed r105057: <http://trac.webkit.org/changeset/105057>
Comment 62 Nikolas Zimmermann 2012-01-16 05:50:30 PST
Created attachment 122622 [details]
Patch chunk 4, v1
Comment 63 Zoltan Herczeg 2012-01-16 06:00:03 PST
Comment on attachment 122622 [details]
Patch chunk 4, v1

Images looks the same. rs=me
Comment 64 Nikolas Zimmermann 2012-01-16 06:04:49 PST
Committed r105061: <http://trac.webkit.org/changeset/105061>
Comment 65 Nikolas Zimmermann 2012-01-16 09:03:31 PST
Created attachment 122649 [details]
Final patch chunk, v1
Comment 66 WebKit Review Bot 2012-01-16 23:58:07 PST
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
Comment 67 Zoltan Herczeg 2012-01-17 02:18:00 PST
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?
Comment 68 Nikolas Zimmermann 2012-01-17 02:50:01 PST
(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.
Comment 69 Zoltan Herczeg 2012-01-17 04:13:27 PST
Comment on attachment 122649 [details]
Final patch chunk, v1

r=me
Probably some layout tests will fail so please check the bots after landing.
Comment 70 Nikolas Zimmermann 2012-01-17 04:42:53 PST
Committed r105143: <http://trac.webkit.org/changeset/105143>
Comment 71 Nikolas Zimmermann 2012-01-17 05:03:28 PST
(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.
Comment 72 Nikolas Zimmermann 2012-01-17 05:10:18 PST
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.
Comment 73 Csaba Osztrogonác 2012-01-17 08:28:14 PST
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
Comment 74 Nikolas Zimmermann 2012-02-08 01:55:24 PST
(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?
Comment 75 Eric Seidel (no email) 2012-02-10 10:19:22 PST
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.
Comment 76 Eric Seidel (no email) 2012-02-10 10:19:28 PST
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.
Comment 77 Eric Seidel (no email) 2012-02-10 10:19:33 PST
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.
Comment 78 Eric Seidel (no email) 2012-02-10 10:19:45 PST
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.
Comment 79 Ahmad Saleem 2023-05-27 16:01:13 PDT
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?