[webkit-reviews] review granted: [Bug 179118] [Experiment] Implement code to detect high frequency painting : [Attachment 325724] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 179118: [Experiment] Implement code to detect high frequency painting
https://bugs.webkit.org/show_bug.cgi?id=179118

Attachment 325724: Patch

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




--- Comment #19 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 325724
  --> https://bugs.webkit.org/attachment.cgi?id=325724
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?