Bug 65711

Created attachment 102958 [details] repro

Created attachment 102959 [details] sample on r92290

<rdar://problem/9898636>

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.

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

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

(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...

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.

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.

(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 :-)

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.

(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 :-)

(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.

Created attachment 120942 [details] Patch chunk 1, v1

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 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 on attachment 120942 [details] Patch chunk 1, v1 Attachment 120942 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11043305

Created attachment 120945 [details] Patch chunk 1, v2

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 on attachment 120945 [details] Patch chunk 1, v2 Attachment 120945 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10954478

Created attachment 120947 [details] Patch chunk 1, v3

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 on attachment 120947 [details] Patch chunk 1, v3 Attachment 120947 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11001609

Created attachment 121069 [details] Patch chunk 1, v4

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 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 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?

Created attachment 121813 [details] Patch chunk 1, v5

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 on attachment 121813 [details] Patch chunk 1, v5 Attachment 121813 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11166938

Created attachment 121819 [details] Patch chunk 1, v6

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?

(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.

Created attachment 121846 [details] Patch chunk 1, v7

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

Created attachment 121849 [details] Patch chunk 1, v8

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

Created attachment 121876 [details] Patch chunk 1, v9

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.

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

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

Created attachment 122197 [details] Patch chunk 2, v1

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 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 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 on attachment 122197 [details] Patch chunk 2, v1 Attachment 122197 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11211359

Created attachment 122202 [details] Patch chunk 2, v2

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.

Created attachment 122411 [details] Patch chunk 2, v3

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.

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

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

Created attachment 122605 [details] Patch chunk 3, v1

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 on attachment 122605 [details] Patch chunk 3, v1 Attachment 122605 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11135116

Created attachment 122606 [details] Patch chunk 3, v2

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 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

Created attachment 122608 [details] Patch chunk 3, v3

Comment on attachment 122608 [details] Patch chunk 3, v3 r=me Carefully written patch.

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

Created attachment 122622 [details] Patch chunk 4, v1

Comment on attachment 122622 [details] Patch chunk 4, v1 Images looks the same. rs=me

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

Created attachment 122649 [details] Final patch chunk, v1

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 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?

(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 on attachment 122649 [details] Final patch chunk, v1 r=me Probably some layout tests will fail so please check the bots after landing.

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

(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.

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.

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

(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 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 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 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 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.

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?