We reinitialize segment iterator for the same content each time TextIterator iterates through the text nodes.
Created attachment 250413 [details] Patch
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.
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);
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/).
Created attachment 250445 [details] Patch
(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.
Comment on attachment 250445 [details] Patch Clearing flags on attachment: 250445 Committed r182604: <http://trac.webkit.org/changeset/182604>
All reviewed patches have been landed. Closing bug.
(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
(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.
AFAIK the problem is that a performance test isn't allowed to produce any output.
(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.