Issue 1160623002: Avoid resetting the metrics-list/character offset for each text box

Issue 1160623002: Avoid resetting the metrics-list/character offset for each text box (Closed)

Created:
5 years, 7 months ago by fs

Modified:
5 years, 6 months ago

Reviewers:
pdr., f(malita)

CC:
blink-reviews, blink-reviews-rendering, krit, eae+blinkwatch, f(malita), fs, gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Avoid resetting the metrics-list/character offset for each text box SVGTextLayoutEngine::layoutTextOnLineOrPath needs to keep track of the SVGTextMetrics corresponding to each glyph it layouts. To do this, the members m_visual{Character,MetricsList}Offset are used. However, this state is reset for each invocation, which means that finding the starting metrics entry can yield O(N^2) behavior. For text nodes with many (logically consecutive) text boxes, this can become noticeable. Introduce a helper SVGTextMetricsIterator - replacing the m_visual* members - and let it persist across calls to layoutTextOnLineOrPath. This speeds up the case of many logically consecutive text boxes. Using the PerfTestRunner.measurePageLoadTime harness with separate-x.svg from the bug give the following results locally: Before: After: avg 101.5 ms -> 93.1 ms median 101.1 ms -> 93.1 ms stdev 2.8 ms -> 2.3 ms BUG=486669 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196054

Patch Set 1 #

Total comments: 16

Patch Set 2 : Shake, rattle and roll. #

Patch Set 3 : Rebase. #

Created: 5 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+77 lines, -66 lines)			Patch
M	Source/core/layout/svg/LayoutSVGInlineText.h	View	1	1 chunk	+51 lines, -0 lines	0 comments	Download
M	Source/core/layout/svg/SVGTextLayoutEngine.h	View	1 2	4 chunks	+3 lines, -7 lines	0 comments	Download
M	Source/core/layout/svg/SVGTextLayoutEngine.cpp	View	1 2	13 chunks	+23 lines, -59 lines	0 comments	Download

Messages

Total messages: 10 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

f(malita)

Nice, LGTM w/ nits. https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGTextLayoutEngine.cpp File Source/core/layout/svg/SVGTextLayoutEngine.cpp (right): https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGTextLayoutEngine.cpp#newcode338 Source/core/layout/svg/SVGTextLayoutEngine.cpp:338: const Vector<SVGTextMetrics>* metricsList = &textLayoutObject.layoutAttributes()->textMetricsValues(); ...

5 years, 7 months ago (2015-05-27 01:53:50 UTC) #3

pdr.

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGTextLayoutEngine.cpp File Source/core/layout/svg/SVGTextLayoutEngine.cpp (right): https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGTextLayoutEngine.cpp#newcode310 Source/core/layout/svg/SVGTextLayoutEngine.cpp:310: void SVGTextLayoutEngine::finishLayout() m_metricsList feels like a use after free ...

5 years, 7 months ago (2015-05-27 03:11:18 UTC) #4

Code-shuffled somewhat in accordance with suggestions. https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGTextLayoutEngine.cpp File Source/core/layout/svg/SVGTextLayoutEngine.cpp (right): https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGTextLayoutEngine.cpp#newcode310 Source/core/layout/svg/SVGTextLayoutEngine.cpp:310: void SVGTextLayoutEngine::finishLayout() On ...

5 years, 7 months ago (2015-05-27 12:53:14 UTC) #5

Code-shuffled somewhat in accordance with suggestions.

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
File Source/core/layout/svg/SVGTextLayoutEngine.cpp (right):

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextLayoutEngine.cpp:310: void
SVGTextLayoutEngine::finishLayout()
On 2015/05/27 03:11:18, pdr wrote:
> m_metricsList feels like a use after free waiting to happen. If we need it,
can
> you clear it here? Maybe m_visualMetricsIterator.clear() or just use an OwnPtr
> to store the iterator and free it here.

I'm not too worried about life-times myself - if the scope of
SVGTextLayoutEngine would be prolonged, there'd be plenty of other reasons to
worry. Added a "clearing" of the iterator here.

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextLayoutEngine.cpp:338: const
Vector<SVGTextMetrics>* metricsList =
&textLayoutObject.layoutAttributes()->textMetricsValues();
On 2015/05/27 01:53:49, f(malita) wrote:
> Let's keep the prev ASSERT(!metricsList->isEmpty()).

Added.

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextLayoutEngine.cpp:339: if
(&m_visualMetricsIterator.metricsList() != metricsList)
On 2015/05/27 03:11:18, pdr wrote:
> Instead of relying on the metrics list address, can we just store an
> LayoutSVGInlineText pointer and rely on it changing? This would mean changing
> the iterator slightly to be something like SVGInlineTextVisualMetricsIterator.

Done, except I left the "Visual" part out (it's confusing enough in the context
of SVGTextLayoutEngine, and there's no other metrics to confuse it with.)

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextLayoutEngine.cpp:421: updateMetricsIterator(text,
textBox->start());
On 2015/05/27 03:11:18, pdr wrote:
> Do you think something like m_visualMetricsIterator.advanceToTextStart(text,
> textBox->start()) might be more clear?

Done.

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
File Source/core/layout/svg/SVGTextLayoutEngine.h (right):

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextLayoutEngine.h:69: void
updateMetricsIterator(const LayoutSVGInlineText&, unsigned characterOffset);
On 2015/05/27 03:11:18, pdr wrote:
> Can you move this to the iterator? It seems to be an implementation detail of
> how the iterator gets updated/advanced.

Done.

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
File Source/core/layout/svg/SVGTextMetrics.h (right):

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextMetrics.h:60: class SVGTextMetricsIterator {
On 2015/05/27 01:53:49, f(malita) wrote:
> nit: can this be a private nested class in SVGTextLayoutEngine?

I have other possible uses in mind for this (SVGTextQuery), that would make that
a poor fit... (Not that this is a great fit, I also pondered putting this in
LayoutSVGInlineText, and given pdr's feedback maybe I should.) Ended up moving
it to LayoutSVGInlineText.h

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextMetrics.h:64: void reset(const
Vector<SVGTextMetrics>* metricsList)
On 2015/05/27 03:11:18, pdr wrote:
> I think this can be an internal detail of the iterator.

I wanted to keep the interface of the iterator fairly low-level to avoid hiding
too much semantics in it (and to keep it simple and default-inlineable.)

https://codereview.chromium.org/1160623002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextMetrics.h:77: const SVGTextMetrics& metrics()
const { return metricsList()[m_metricsListOffset]; }
On 2015/05/27 01:53:49, f(malita) wrote:
> nit: ASSERT(m_metricsList && m_metricsListOffset < m_metricsList->size());?

I've sort of been relying on the RELEASE_ASSERT in operator[] here - that and a
nullptr crash... Added as requested though.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1160623002/40001

5 years, 6 months ago (2015-05-28 11:17:47 UTC) #9

commit-bot: I haz the power

5 years, 6 months ago (2015-05-28 11:21:22 UTC) #10

Message was sent while issue was closed.

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196054

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages