Bug 215989 – REGRESSION(r262366): MotionMark1.1 | macOS | Some devices | 1-3% overall regression

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

REGRESSION(r262366): MotionMark1.1 | macOS | Some devices | 1-3% overall regression

[Dean Jackson]

<rdar://problem/66845937>

[Dean Jackson]

Created attachment 407571 [details]
Patch

[Darin Adler]

Comment on attachment 407571 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8639
> +    if (m_canvasesNeedingDisplayPreparation.isEmpty())
> +        return;

This isn’t needed. The code below already efficiently does nothing in this case. Making an empty vector, an empty for loop, and an empty clear function are all efficient.

> Source/WebCore/html/CanvasBase.cpp:140
> +    if (m_observers.isEmpty())
> +        return;

This isn’t needed. The code below already efficiently does nothing in this case. Making an empty vector and an empty for loop are efficient.

> Source/WebCore/html/CanvasBase.cpp:149
> +    if (m_observers.isEmpty())
> +        return;

This isn’t needed. The code below already efficiently does nothing in this case. Making an empty vector and an empty for loop are efficient.

> Source/WebCore/html/CanvasBase.cpp:162
> +    if (m_observers.isEmpty()) {
> +#if ASSERT_ENABLED
> +        m_didNotifyObserversCanvasDestroyed = true;
> +#endif
> +        return;
> +    }

This isn’t needed. The code below already efficiently does nothing in this case. Making an empty vector, an empty for loop, and an empty clear function are all efficient.

> Source/WebCore/html/HTMLCanvasElement.cpp:1054
> +    if (needsPreparationForDisplay()) {
> +        if (insertionType.connectedToDocument) {

I suggest && rather than a nested if statement.

> Source/WebCore/html/HTMLCanvasElement.cpp:1055
> +            auto& doc = parentOfInsertedTree.document();

Please use document, not doc.

> Source/WebCore/html/HTMLCanvasElement.cpp:1060
> +            if (is<WebGLRenderingContextBase>(m_context.get())) {
> +                if (downcast<WebGLRenderingContextBase>(m_context.get())->compositingResultsNeedUpdating())

Three thoughts:

1) Instead of "(m_context.get())->" you can use "(*m_context)." and this applies here and other places in the file.
2) Instead of nested if you can use &&, which I think is clearer, but it ends up being a long line because of repeating the WebGLRenderingContextBase class name twice.
3) Could make compositingResultsNeedUpdating a virtual function and avoid the type check entirely.

> Source/WebCore/html/HTMLCanvasElement.cpp:1072
> +    if (needsPreparationForDisplay()) {
> +        if (removalType.disconnectedFromDocument) {

I suggest && rather than a nested if statement.

> Source/WebCore/html/HTMLCanvasElement.cpp:1084
> +    if (!m_context)
> +        return false;

This isn’t needed. The code below already efficiently returns false when m_context is null.

[Dean Jackson]

I forgot to say that I measured and this does recover the 1-3% slowdown.

[Dean Jackson]

Thanks Darin! I'll make all the suggested changes.
(and update the changelog to note that I measured performance).

As for the empty functions, I obviously didn't look at the implementation, but I assumed that the copyToVector(empty-HashSet) would have to actually create a Vector in order for the loop to know it was empty. I assume that's so insignificant it doesn't matter?

[Darin Adler]

(In reply to Dean Jackson from comment #5)
> I assumed that the copyToVector(empty-HashSet) would have to actually
> create a Vector in order for the loop to know it was empty. I assume that's
> so insignificant it doesn't matter?

Yes, it has to create a vector.

But a 0-sized vector is all on the stack (not the heap) and creating one should not involve any memory allocation and should not be all that different from checking the set for emptiness.

[Dean Jackson]

Created attachment 407640 [details]
Trace (before patch)

If I look at the attached trace (which is without this patch), and search for CanvasBase::notifyObserversCanvasChanged I see copyToVector allocating a buffer which calls into bmalloc.

[Dean Jackson]

Created attachment 407642 [details]
Instruments screenshot

[Darin Adler]

OK, but that’s not an empty set.

[Darin Adler]

Is this called with non-empty sets too during the test?

[Darin Adler]

Do we really need to copy the vector every time? Can the canvasChanged functions mutate the observers set?

[Dean Jackson]

Oh. I'm a moron!

The whole point of this patch was to make that HashSet empty in this case. It isn't empty before the fix.

[Dean Jackson]

Ha. Darin guessed it as I worked it out.

[Dean Jackson]

> Do we really need to copy the vector every time? Can the canvasChanged functions mutate the observers set?

The answer is yes, they technically can.

I thought about different ways to avoid this. Right now the vectors don't change because I control the calls inside both the canvas observer updates and the after rendering updates, but someone could something bad if they didn't know.

Maybe I could pass const CanvasBases around, but the called functions still need to be non-const.

[Dean Jackson]

I can't pass const CanvasBase, because they ultimately need to call a non-const.

But yeah, a canvasChanged call should never mutate the set of observers on itself.

[Dean Jackson]

"notifyObserversCanvasDestroyed" might though, since the notification might do something to remove itself as an observer (even though it's already being deleted).

[Dean Jackson]

> 3) Could make compositingResultsNeedUpdating a virtual function and avoid the type check entirely.

I made compositingResultsNeedUpdating, prepareForDisplay and needsPreparationForDisplay all virtuals, which cleans up the code significantly.

[Dean Jackson]

Committed r266443: <https://trac.webkit.org/changeset/266443>