| Summary: | Simple line layout(regression): Calling innerText on RenderFlow with multiple children is slow. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | zalan <zalan> | ||||||
| Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, koivisto, ossy, rniwa | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
zalan
2015-04-08 20:46:35 PDT
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. |