Bug 216677 - REGRESSION(r267137): PaintFrequencyTracker needs to track all the painting
Summary: REGRESSION(r267137): PaintFrequencyTracker needs to track all the painting
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-17 18:36 PDT by Said Abou-Hallawa
Modified: 2020-09-18 12:30 PDT (History)
11 users (show)

See Also:


Attachments
Patch (2.22 KB, patch)
2020-09-17 18:46 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (5.89 KB, patch)
2020-09-18 11:06 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 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.
Comment 1 Said Abou-Hallawa 2020-09-17 18:37:11 PDT
<rdar://problem/69080461>
Comment 2 Said Abou-Hallawa 2020-09-17 18:46:32 PDT
Created attachment 409098 [details]
Patch
Comment 3 Simon Fraser (smfr) 2020-09-17 18:53:01 PDT
Comment on attachment 409098 [details]
Patch

What painting does not "come through" Page::updateRendering() ?
Comment 4 Said Abou-Hallawa 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)
Comment 5 Simon Fraser (smfr) 2020-09-17 19:14:21 PDT
But none of those code paths should be performance-sensitive. updateControlTints shouldn't even paint glyph buffers.
Comment 6 Said Abou-Hallawa 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:]
Comment 7 Said Abou-Hallawa 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.
Comment 8 Simon Fraser (smfr) 2020-09-18 09:13:56 PDT
That makes more sense. Be sure to check iOS and macOS which paint differently.
Comment 9 Said Abou-Hallawa 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.
Comment 10 Said Abou-Hallawa 2020-09-18 11:06:18 PDT
Created attachment 409149 [details]
Patch
Comment 11 EWS 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].