Bug 143554 – Simple line layout(regression): Calling innerText on RenderFlow with multiple children is slow.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

We reinitialize segment iterator for the same content each time TextIterator iterates through the text nodes.

[alan]

Created attachment 250413 [details]
Patch

[Antti Koivisto]

Comment on attachment 250413 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:558
> +            if (previousRenderer.parent() != &blockFlow) {
> +                m_runResolver = adoptPtr(new SimpleLineLayout::RunResolver(blockFlow, *layout));

Wouldn't it be clearer to initialise the cache when entering a flow with simple line layout (and clear when leaving it)? Now we leave an unneeded RunResolver behind (not much of a practical problem I suppose).

> Source/WebCore/editing/TextIterator.cpp:564
> +        if (!m_runResolver)
> +            m_runResolver = adoptPtr(new SimpleLineLayout::RunResolver(blockFlow, *layout));

Why do we need two separate places to initialize this?

How about

if (!m_runResolver || !&m_runResolver->flow() != &blockFlow)

(or the suggestion above).

> Source/WebCore/editing/TextIterator.h:172
> +    OwnPtr<SimpleLineLayout::RunResolver> m_runResolver;

OwnPtr is dead. Please use std::unique_ptr instead.

This could be called something more informative, m_flowRunResolverCache or similar.

> Source/WebCore/rendering/SimpleLineLayoutFlowContents.cpp:43
> +    for (const auto& child : childrenOfType<RenderText>(flow)) {
> +        ASSERT_UNUSED(child, &child);
> +        ++numberOfChildren;
> +    }

The assert doesn't really add any value.

[Antti Koivisto]

Comment on attachment 250413 [details]
Patch

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

>> Source/WebCore/editing/TextIterator.cpp:558
>> +                m_runResolver = adoptPtr(new SimpleLineLayout::RunResolver(blockFlow, *layout));
> 
> Wouldn't it be clearer to initialise the cache when entering a flow with simple line layout (and clear when leaving it)? Now we leave an unneeded RunResolver behind (not much of a practical problem I suppose).

std::make_unique<SimpleLineLayout::RunResolver>(blockFlow, *layout);

[Antti Koivisto]

Comment on attachment 250413 [details]
Patch

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

> PerformanceTests/ChangeLog:11
> +        * Layout/simple-line-layout-innertext.html: Added.

For clear O(n^2) bugs like this it might be better to make a magnitude test (LayoutTests/perf/).

[alan]

Created attachment 250445 [details]
Patch

[alan]

(In reply to comment #2)
> Comment on attachment 250413 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250413&action=review
> 
> > Source/WebCore/editing/TextIterator.cpp:558
> > +            if (previousRenderer.parent() != &blockFlow) {
> > +                m_runResolver = adoptPtr(new SimpleLineLayout::RunResolver(blockFlow, *layout));
> 
> Wouldn't it be clearer to initialise the cache when entering a flow with
> simple line layout (and clear when leaving it)? Now we leave an unneeded
> RunResolver behind (not much of a practical problem I suppose).
> 
> > Source/WebCore/editing/TextIterator.cpp:564
> > +        if (!m_runResolver)
> > +            m_runResolver = adoptPtr(new SimpleLineLayout::RunResolver(blockFlow, *layout));
> 
> Why do we need two separate places to initialize this?
> 
> How about
> 
> if (!m_runResolver || !&m_runResolver->flow() != &blockFlow)
> 
Fixed.

> (or the suggestion above).
> 
> > Source/WebCore/editing/TextIterator.h:172
> > +    OwnPtr<SimpleLineLayout::RunResolver> m_runResolver;
> 
> OwnPtr is dead. Please use std::unique_ptr instead.
Fixed.

> 
> This could be called something more informative, m_flowRunResolverCache or
> similar.
Fixed.

> 
> > Source/WebCore/rendering/SimpleLineLayoutFlowContents.cpp:43
> > +    for (const auto& child : childrenOfType<RenderText>(flow)) {
> > +        ASSERT_UNUSED(child, &child);
> > +        ++numberOfChildren;
> > +    }
> 
> The assert doesn't really add any value.
Fixed.

[WebKit Commit Bot]

Comment on attachment 250445 [details]
Patch

Clearing flags on attachment: 250445

Committed r182604: <http://trac.webkit.org/changeset/182604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #7)
> Comment on attachment 250445 [details]
> Patch
> 
> Clearing flags on attachment: 250445
> 
> Committed r182604: <http://trac.webkit.org/changeset/182604>

The new test fails on the performance bots:

Running Layout/simple-line-layout-innertext.html (110 of 138)
ERROR: foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar
FAILED
Finished: 7.527268 s

[alan]

(In reply to comment #9)
> (In reply to comment #7)
> > Comment on attachment 250445 [details]
> > Patch
> > 
> > Clearing flags on attachment: 250445
> > 
> > Committed r182604: <http://trac.webkit.org/changeset/182604>
> 
> The new test fails on the performance bots:
> 
> Running Layout/simple-line-layout-innertext.html (110 of 138)
> ERROR:
> foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfooba
> rfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoob
> arfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoo
> barfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfo
> obarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarf
> oobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar
> foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfooba
> rfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoob
> arfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoo
> barfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfo
> obarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarf
> oobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar
> foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfooba
> rfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoob
> arfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoo
> barfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfo
> obarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarf
> oobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar
> foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfooba
> rfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoob
> arfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoo
> barfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfo
> obarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarf
> oobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar
> foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfooba
> rfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoob
> arfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoo
> barfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfo
> obarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarf
> oobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar
> foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfooba
> rfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoob
> arfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoo
> barfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfo
> obarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarf
> oobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar
> foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfooba
> rfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoob
> arfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar
> FAILED
> Finished: 7.527268 s
Thanks, I'll talk to Ryosuke about it.

[Csaba Osztrogonác]

AFAIK the problem is that a performance test isn't allowed to produce any output.

[alan]

(In reply to comment #11)
> AFAIK the problem is that a performance test isn't allowed to produce any
> output.
Yea, that's an odd limitation. -and that's what I was planning to discuss with Ryosuke.