Bug 215989

<rdar://problem/66845937>

Created attachment 407571 [details] Patch

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.

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

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?

(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.

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.

Created attachment 407642 [details] Instruments screenshot

OK, but that’s not an empty set.

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

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

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.

Ha. Darin guessed it as I worked it out.

> 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.

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.

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

> 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.

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