Bug 216677

Summary: REGRESSION(r267137): PaintFrequencyTracker needs to track all the painting
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: Layout and RenderingAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, esprehn+autocc, ews-watchlist, fred.wang, glenn, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Said Abou-Hallawa
Reported 2020-09-17 18:36:34 PDT
r267137 made PaintFrequencyTracker track only the painting which comes through Page::updateRendering(). This caused ~30% regression in the MotionMark Design test because it enlarged the gap between the paintings. So PaintFrequencyTracker will never encounter a PaintFrequency::High. This means we lost the optimization that could be gained from replaying back the DisplayList of the text runs.
Attachments
Patch (2.22 KB, patch)
2020-09-17 18:46 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (5.89 KB, patch)
2020-09-18 11:06 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2020-09-17 18:37:11 PDT
<rdar://problem/69080461>
Said Abou-Hallawa
Comment 2 2020-09-17 18:46:32 PDT
Created attachment 409098 [details] Patch
Simon Fraser (smfr)
Comment 3 2020-09-17 18:53:01 PDT
Comment on attachment 409098 [details] Patch What painting does not "come through" Page::updateRendering() ?
Said Abou-Hallawa
Comment 4 2020-09-17 19:01:43 PDT
Updating control tints: See the call stack in https://bugs.webkit.org/show_bug.cgi?id=216190#c10. Or WebPage snapshotting: WebCore: WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) WebCore: WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) WebCore: WebCore::RenderLayer::paintLayerWithEffects(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) WebCore: WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) WebCore: WebCore::RenderLayer::paintList(WebCore::RenderLayer::LayerList, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) WebCore: WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) WebCore: WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) WebCore: WebCore::RenderLayer::paintLayerWithEffects(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) WebCore: WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) WebCore: WebCore::RenderLayer::paint(WebCore::GraphicsContext&, WebCore::LayoutRect const&, WebCore::LayoutSize const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::RenderObject*, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>, WebCore::RenderLayer::SecurityOriginPaintPolicy, WebCore::EventRegionContext*) WebCore: WebCore::FrameView::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&, WebCore::Widget::SecurityOriginPaintPolicy, WebCore::EventRegionContext*) WebCore: WebCore::FrameView::paintContentsForSnapshot(WebCore::GraphicsContext&, WebCore::IntRect const&, WebCore::FrameView::SelectionInSnapshot, WebCore::FrameView::CoordinateSpaceForSnapshot) WebKit: WebKit::WebPage::paintSnapshotAtSize(WebCore::IntRect const&, WebCore::IntSize const&, unsigned int, WebCore::Frame&, WebCore::FrameView&, WebCore::GraphicsContext&) WebKit: WebKit::WebPage::snapshotAtSize(WebCore::IntRect const&, WebCore::IntSize const&, unsigned int) WebKit: WebKit::WebPage::scaledSnapshotWithOptions(WebCore::IntRect const&, double, unsigned int)
Simon Fraser (smfr)
Comment 5 2020-09-17 19:14:21 PDT
But none of those code paths should be performance-sensitive. updateControlTints shouldn't even paint glyph buffers.
Said Abou-Hallawa
Comment 6 2020-09-18 02:15:18 PDT
#2 WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) #3 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, WebCore::IntRect const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::EventRegionContext*)::$_22::operator()(WebCore::RenderLayer&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) const #4 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, WebCore::IntRect const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::EventRegionContext*) #5 WebCore::RenderLayerBacking::paintContents(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, WebCore::FloatRect const&, unsigned int) #6 WebCore::GraphicsLayer::paintGraphicsLayerContents(WebCore::GraphicsContext&, WebCore::FloatRect const&, unsigned int) #7 WebCore::GraphicsLayerCA::platformCALayerPaintContents(WebCore::PlatformCALayer*, WebCore::GraphicsContext&, WebCore::FloatRect const&, unsigned int) #8 WebCore::PlatformCALayer::drawLayerContents(WebCore::GraphicsContext&, WebCore::PlatformCALayer*, WTF::Vector<WebCore::FloatRect, 5ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, unsigned int) #9 WebCore::TileGrid::platformCALayerPaintContents(WebCore::PlatformCALayer*, WebCore::GraphicsContext&, WebCore::FloatRect const&, unsigned int) #10 [WebSimpleLayer drawInContext:]
Said Abou-Hallawa
Comment 7 2020-09-18 08:42:37 PDT
I was wrong. I understand now what is happening. Resetting the Page::m_renderingUpdateTimestamp at the end of Page::updateRendering() is wrong since this will make RenderLayer::paintLayerContents() sees it always zero. There is no direct call from Page::updateRendering() to RenderLayer::paintLayerContents(). Actual painting is scheduled in next run loop through TiledCoreAnimationDrawingArea::scheduleRenderingUpdateRunLoopObserver(). So PaintFrequencyTracker does not see the gap between the painting increased as I stated above. In fact PaintFrequencyTracker is not tracking any painting because the timestamp is always zero. And this is why we were falling always to the none DisplayList replaying back scenario.
Simon Fraser (smfr)
Comment 8 2020-09-18 09:13:56 PDT
That makes more sense. Be sure to check iOS and macOS which paint differently.
Said Abou-Hallawa
Comment 9 2020-09-18 10:46:56 PDT
I made Page::updateRendering() records the last MonotonicTime::now() when it called and make PaintFrequencyTracker use this timestamp. This is the same as the change in r266677. The only difference is PaintFrequencyTracker will track all the painting from now on. When Page::lastRenderingUpdateTimestamp() is zero, PaintFrequencyTracker will use call MonotonicTime::now() which will happen only in cases like calling updateControlTints() before calling Page::updateRendering() is called.
Said Abou-Hallawa
Comment 10 2020-09-18 11:06:18 PDT
Created attachment 409149 [details] Patch
EWS
Comment 11 2020-09-18 12:30:10 PDT
Committed r267254: <https://trac.webkit.org/changeset/267254> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409149 [details].
Note You need to log in before you can comment on or make changes to this bug.