Bug 179118 – [Experiment] Implement code to detect high frequency painting

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

Implement code to detect high frequency painting.

[Dave Hyatt]

Created attachment 325594 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 325594 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:137
> +#define PAINT_FREQUENCY_FPS_THRESHOLD 30

This should be const unsigned, or maybe a per-frame ms value

> Source/WebCore/rendering/RenderLayer.cpp:138
> +#define PAINT_FREQUENCY_SECONDS_IDLE_THRESHOLD 5

This should be const Seconds (and you can use the 5_s literal form).

> Source/WebCore/rendering/RenderLayer.cpp:6817
> +    MonotonicTime now = MonotonicTime::now();

I think this...

> Source/WebCore/rendering/RenderLayer.cpp:6825
> +bool RenderLayer::paintingFrequently()

It's a bit weird that paintingFrequently() does all this work, and isn't just a simple const accessor. Can we do the "painting frequently" math on the layer at the start or end of painting if it has a m_paintFrequencyInfo?

That way, maybe we can avoid passing 'bool paintingFrequently' too the paint functions.

> Source/WebCore/rendering/RenderLayer.cpp:6830
> +        MonotonicTime now = MonotonicTime::now();

... and this should use the same time captured by AnimationUpdateBlock, so they don't change in the middle of a paint.

> Source/WebCore/rendering/RenderLayer.cpp:6832
> +            m_paintFrequencyInfo.firstPaintTime = m_paintFrequencyInfo.lastPaintTime = MonotonicTime();

We never do multiple assignments on one line. This should also use 'now' (or the snapshotted animation start time).

> Source/WebCore/rendering/RenderLayer.h:1184
> +    PaintFrequencyInfo m_paintFrequencyInfo;

I wonder if PaintFrequencyInfo should be std::optional<>, and only allocated if someone calls notifyPaintHasCachableResources()? Then the "since first" ambiguity goes away, as does what "first paint" means.

> Source/WebCore/testing/Internals.h:105
> +        

Whitespace

> Source/WebCore/testing/Internals.h:108
> +        

Whitespace

> Source/WebCore/testing/Internals.idl:96
> +    // Must be called on an element whose enclosingLayer() is self-painting.

Maybe it should throw an exception if not?

> LayoutTests/fast/block/block-move-frequent-paint.html:25
> +    if (counter < 60)
> +        setTimeout(runTest, 0);

This should use requestAnimationFrame, right?

[Dave Hyatt]

Yeah, good ideas here. If I get the time from another source as you suggest (e.g., AnimationUpdateBlock), then I can avoid creating a PaintFrequencyInfo until someone says they need to cache.

I can put a boolean on the RenderLayer for paintingFrequently as well, and then avoid having to add that to PaintInfo.

[Build Bot]

Comment on attachment 325594 [details]
Patch

Attachment 325594 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5064560

New failing tests:
fast/block/block-move-frequent-paint.html

[Build Bot]

Created attachment 325607 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Dave Hyatt]

Created attachment 325620 [details]
Patch

[Build Bot]

Attachment 325620 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.cpp:4300:  Extra space after ( in if  [whitespace/parens] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Created attachment 325622 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 325622 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:138
> +#define PAINT_FREQUENCY_SECONDS_IDLE_THRESHOLD 5

const Seconds paintFrequencyIdleThreshold = 5_s;

> Source/WebCore/rendering/RenderLayer.cpp:4302
> +            if ((now - m_paintFrequencyInfo->lastPaintTime).seconds() > PAINT_FREQUENCY_SECONDS_IDLE_THRESHOLD) {

paintFrequencyIdleThreshold

> Source/WebCore/rendering/RenderLayer.cpp:6819
> +void RenderLayer::notifyPaintHasCachableResources()

Maybe this should be called "startTrackingPaintFrequency()"?

> Source/WebCore/rendering/RenderLayer.cpp:6834
> +    m_paintFrequencyInfo->totalPaintsSinceFirst++;

Doesn't totalPaintsSinceFirst need to be reset to 0 in some cases, like if you've been painting frequently, then not, then frequently again?

> Source/WebCore/rendering/RenderLayer.h:1035
> +    class PaintFrequencyInfo {

This can be a struct with no need for "public:"

> Source/WebCore/rendering/RenderLayer.h:1127
> +    bool m_paintingFrequently : 1;

I think m_paintingFrequently should be some state inside PaintFrequencyInfo. It's never true if you don't have a PaintFrequencyInfo, right?

[Build Bot]

Comment on attachment 325622 [details]
Patch

Attachment 325622 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5066505

New failing tests:
fast/block/block-move-frequent-paint.html

[Build Bot]

Created attachment 325632 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325622 [details]
Patch

Attachment 325622 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5066594

New failing tests:
fast/block/block-move-frequent-paint.html

[Build Bot]

Created attachment 325638 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Dave Hyatt]

Created attachment 325718 [details]
Patch

[Dave Hyatt]

Created attachment 325719 [details]
Patch

[Build Bot]

Attachment 325719 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.cpp:163:  Too many spaces inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/rendering/RenderLayer.cpp:172:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Created attachment 325724 [details]
Patch

[Dave Hyatt]

I ditched the test, since bots just don't run fast enough to animate anything.

[Simon Fraser (smfr)]

Comment on attachment 325724 [details]
Patch

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

Maybe remove the Internals bits if you don't use them.

> Source/WebCore/rendering/PaintInfo.h:105
> +    RenderLayer* enclosingSelfPaintingLayer() { return m_enclosingSelfPaintingLayer; }

const

> Source/WebCore/rendering/RenderLayer.cpp:152
> +    PaintFrequencyInfo(const MonotonicTime& now)

You can pass MonotonicTime by value; it's just the size of a double.

> Source/WebCore/rendering/RenderLayer.cpp:158
> +    void cachedResourcePainting(const MonotonicTime&);

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:170
> +void PaintFrequencyInfo::layerPainting(RenderLayer* layer)

Maybe just willPaint()

> Source/WebCore/rendering/RenderLayer.cpp:174
> +    MonotonicTime now = MonotonicTime::now(); // FIXME: Should have a single time for the paint of the whole frame.
> +    if ((now - m_lastPaintTime) > paintFrequencySecondsIdleThreshold)
> +        layer->clearPaintFrequencyInfo();

Kinda gross to pass in the layer and do the clearing like this. How about returning an enum:

enum class PaintFrequency { Idle, Low, High } and if the return value is PaintFrequency::Idle then the caller clears m_paintFrequencyInfo

> Source/WebCore/rendering/RenderLayer.cpp:175
> +    else if ((m_totalPaints / (now - m_firstPaintTime).seconds()) > PAINT_FREQUENCY_FPS_THRESHOLD)

Would be slightly nicer to flip the math and do mean ms per frame, so you could replace PAINT_FREQUENCY_FPS_THRESHOLD with a 32_ms value.

> Source/WebCore/rendering/RenderLayer.cpp:6860
> +    return m_paintFrequencyInfo->paintingFrequently();

Maybe return m_paintFrequencyInfo && m_paintFrequencyInfo->paintingFrequently()

> Source/WebCore/rendering/RenderLayer.cpp:6869
> +        m_paintFrequencyInfo->cachedResourcePainting(now);

This "cachedResource" terminology is a bit weird here. First, it's a bit close to the actual CachedResource class. Second, we may want to track paint frequency for other reasons (e.g. compositing).

If you have a PaintFrequencyInfo, why isn't m_totalPaints etc. incremented in willPaint()? Not sure why willPaint() and cachedResourcePainting() can't be combined.

> Source/WebCore/rendering/RenderLayer.cpp:6875
> +    if (m_paintFrequencyInfo)
> +        m_paintFrequencyInfo.reset();

This could just be m_paintFrequencyInfo = nullptr.

> Source/WebCore/testing/Internals.idl:99
> +    boolean isPaintingFrequently(Element element);
> +    void incrementFrequentPaintCounter(Element element);
> +

Is there a test that uses these?

[Radar WebKit Bug Importer]

<rdar://problem/35347068>

[Dave Hyatt]

(In reply to Simon Fraser (smfr) from comment #19)

> If you have a PaintFrequencyInfo, why isn't m_totalPaints etc. incremented
> in willPaint()? Not sure why willPaint() and cachedResourcePainting() can't
> be combined.
>

This is because a layer can paint without actually painting any cacheable resources. For example, something in the upper left corner of a layer could be animating, while the thing we might want to cache isn't painting at all. You don't want to consider the layer to be "frequently painting" unless it is painting something that needs caching.

By using a callback from the cacheable resource, you ensure that you only go into this mode while frequently painting these resources (and not being fooled into entering the mode for no reason).

[Dave Hyatt]

Created attachment 326128 [details]
Patch

[Build Bot]

Attachment 326128 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.cpp:174:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 326128 [details]
Patch

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

> Source/WebCore/rendering/PaintInfo.h:105
> +    const RenderLayer* enclosingSelfPaintingLayer() { return m_enclosingSelfPaintingLayer; }

I meant the function should be const

> Source/WebCore/rendering/RenderLayer.cpp:176
> +    else if (m_totalPaints && ((now - m_firstPaintTime) / m_totalPaints) > paintFrequencyTimePerFrameThreshold) {

no else after return.

[Build Bot]

Comment on attachment 326128 [details]
Patch

Attachment 326128 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5124250

New failing tests:
fast/block/block-move-frequent-paint.html

[Build Bot]

Created attachment 326132 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326128 [details]
Patch

Attachment 326128 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5124346

New failing tests:
fast/block/block-move-frequent-paint.html

[Build Bot]

Created attachment 326137 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326128 [details]
Patch

Attachment 326128 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5124369

New failing tests:
http/tests/workers/service/service-worker-clear.html

[Build Bot]

Created attachment 326138 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Dave Hyatt]

Created attachment 326142 [details]
Patch

[Build Bot]

Comment on attachment 326142 [details]
Patch

Attachment 326142 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5125407

New failing tests:
fast/block/block-move-frequent-paint.html

[Build Bot]

Created attachment 326150 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326142 [details]
Patch

Attachment 326142 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5125468

New failing tests:
fast/block/block-move-frequent-paint.html

[Build Bot]

Created attachment 326152 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Dave Hyatt]

Created attachment 326220 [details]
Patch

Trying again with a massive debug fudge. If this still doesn't work, I'm just going to give up on a test.

[Build Bot]

Comment on attachment 326220 [details]
Patch

Attachment 326220 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5136311

New failing tests:
fast/block/block-move-frequent-paint.html

[Build Bot]

Created attachment 326223 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326220 [details]
Patch

Attachment 326220 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5136360

New failing tests:
fast/block/block-move-frequent-paint.html

[Build Bot]

Created attachment 326229 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Dave Hyatt]

Created attachment 326237 [details]
Patch

[Dave Hyatt]

Landed in 224582.