Bug 227229

<rdar://79147947>

Created attachment 431899 [details] Patch

Comment on attachment 431899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431899&action=review > Source/WebKit/ChangeLog:18 > + corresponding entries form m_fontIdentifierToLastRenderingUpdateVersionMap. typo: "form" > Source/WebKit/ChangeLog:21 > + 3) We need to prepareForNextRenderingUpdate() even if no font will be > + removed from m_fontIdentifierToLastRenderingUpdateVersionMap. prepareForNextRenderingUpdate() is a new function that's added by this patch, so I don't quite understand why this list in the ChangeLog includes a detail of its calling pattern. > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:137 > + m_currentRenderingUpdateVersion = 0; RemoteResourceCacheProxy::cacheFont() has: auto result = m_fontIdentifierToLastRenderingUpdateVersionMap.ensure(font.renderingResourceIdentifier(), [&] { return 0; }); auto& lastVersion = result.iterator->value; if (lastVersion != m_currentRenderingUpdateVersion) { lastVersion = m_currentRenderingUpdateVersion; ++m_numberOfFontsUsedInCurrentRenderingUpdate; } If we set m_currentRenderingUpdateVersion to 0 here, then this "if" block will not execute for new fonts that enter cacheFont() after clearFontMap() runs, so we would end up telling the GPUP to cache the new font without incrementing m_numberOfFontsUsedInCurrentRenderingUpdate. > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:149 > + if (!unusedFontCount || unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount) { It looks like you changed the && to ||. Was that intentional? If || is intentional, I think we can just simplify the entire check to "if (unusedFontCount <= minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount)" If not, and it's supposed to be && instead, then I think the check can be just "if (!unusedFontCount)" > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:150 > + prepareForNextRenderingUpdate(); I don't know if the ChangeLog mentions this, but this seems pretty important. Previously, we weren't incrementing m_currentRenderingUpdateVersion in this branch, which means we were treating adjacent rendering updates as not-distinct if we felt that we didn't need to prune m_fontIdentifierToLastRenderingUpdateVersionMap. Now, with this patch, even if we feel that we don't need to prune m_fontIdentifierToLastRenderingUpdateVersionMap, we're treating adjacent rendering updates as distinct. I think this is important and valuable, and probably worth mentioning in the ChangeLog. > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:154 > + Vector<RenderingResourceIdentifier> toBeRemoved; What's the purpose of the vector? Why not put the call to m_fontIdentifierToLastRenderingUpdateVersionMap.remove() inside the for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap) loop? Usually we use these vectors to guarantee lifetimes of stuff using RefPtrs, but RenderingResourceIdentifier doesn't guarantee any lifetimes. > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:164 > + for (auto& renderingResourceIdentifier : toBeRemoved) Identifiers are just single scalar values, so I think "auto" is better than "auto&"

Comment on attachment 431899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431899&action=review >> Source/WebKit/ChangeLog:18 >> + corresponding entries form m_fontIdentifierToLastRenderingUpdateVersionMap. > > typo: "form" Fixed. >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:137 >> + m_currentRenderingUpdateVersion = 0; > > RemoteResourceCacheProxy::cacheFont() has: > > auto result = m_fontIdentifierToLastRenderingUpdateVersionMap.ensure(font.renderingResourceIdentifier(), [&] { > return 0; > }); > auto& lastVersion = result.iterator->value; > if (lastVersion != m_currentRenderingUpdateVersion) { > lastVersion = m_currentRenderingUpdateVersion; > ++m_numberOfFontsUsedInCurrentRenderingUpdate; > } > > If we set m_currentRenderingUpdateVersion to 0 here, then this "if" block will not execute for new fonts that enter cacheFont() after clearFontMap() runs, so we would end up telling the GPUP to cache the new font without incrementing m_numberOfFontsUsedInCurrentRenderingUpdate. You are right. This can be fixed by changing the ensure functor to be: auto result = m_fontIdentifierToLastRenderingUpdateVersionMap.ensure(font.renderingResourceIdentifier(), [&] { m_remoteRenderingBackendProxy.cacheFont(makeRef(font)); ++m_numberOfFontsUsedInCurrentRenderingUpdate; return m_currentRenderingUpdateVersion; }); >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:149 >> + if (!unusedFontCount || unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount) { > > It looks like you changed the && to ||. Was that intentional? > > If || is intentional, I think we can just simplify the entire check to "if (unusedFontCount <= minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount)" > > If not, and it's supposed to be && instead, then I think the check can be just "if (!unusedFontCount)" It was intentional because, as you said, the above if-statement is equivalent to "if (!unusedFontCount)". I added "!unusedFontCount" to avoid the multiplication in the clause "minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount". But I will change it as you suggested since you think it will be clearer. >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:150 >> + prepareForNextRenderingUpdate(); > > I don't know if the ChangeLog mentions this, but this seems pretty important. Previously, we weren't incrementing m_currentRenderingUpdateVersion in this branch, which means we were treating adjacent rendering updates as not-distinct if we felt that we didn't need to prune m_fontIdentifierToLastRenderingUpdateVersionMap. Now, with this patch, even if we feel that we don't need to prune m_fontIdentifierToLastRenderingUpdateVersionMap, we're treating adjacent rendering updates as distinct. I think this is important and valuable, and probably worth mentioning in the ChangeLog. A comment will be added to the ChangeLog. >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:154 >> + Vector<RenderingResourceIdentifier> toBeRemoved; > > What's the purpose of the vector? Why not put the call to m_fontIdentifierToLastRenderingUpdateVersionMap.remove() inside the for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap) loop? Usually we use these vectors to guarantee lifetimes of stuff using RefPtrs, but RenderingResourceIdentifier doesn't guarantee any lifetimes. I was using it because we loop through the items of m_fontIdentifierToLastRenderingUpdateVersionMap using the modern C++ for-loop which, I think, is not suitable for deleting items while looping. I will replace it by calling HashMap::removeIf() instead.

Created attachment 431941 [details] Patch

Comment on attachment 431899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431899&action=review >>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:154 >>> + Vector<RenderingResourceIdentifier> toBeRemoved; >> >> What's the purpose of the vector? Why not put the call to m_fontIdentifierToLastRenderingUpdateVersionMap.remove() inside the for (auto& item : m_fontIdentifierToLastRenderingUpdateVersionMap) loop? Usually we use these vectors to guarantee lifetimes of stuff using RefPtrs, but RenderingResourceIdentifier doesn't guarantee any lifetimes. > > I was using it because we loop through the items of m_fontIdentifierToLastRenderingUpdateVersionMap using the modern C++ for-loop which, I think, is not suitable for deleting items while looping. I will replace it by calling HashMap::removeIf() instead. Cooooool!

Committed r279104 (239021@main): <https://commits.webkit.org/239021@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431941 [details].

(In reply to EWS from comment #7) > Committed r279104 (239021@main): <https://commits.webkit.org/239021@main> This seems to have made this test very flaky on iOS and mac wk2: fast/canvas/canvas-overloads-strokeText.html History: https://results.webkit.org/?suite=layout-tests&test=fast%2Fcanvas%2Fcanvas-overloads-strokeText.html

It looks like the test is failing because the GPUProcess terminates the WebProcess. I replaced the macro TERMINATE_WEB_PROCESS_WITH_MESSAGE() with Assert(false) and I ran the following command: run-webkit-tests --debug --no-retry LayoutTests/fast/canvas I got the following result: fast/canvas/canvas-overloads-strokeText.html [ Crash ] And I got the following call stack when the assertion fired in the GPUProcess: ASSERTION FAILED: false /Volumes/Data/WebKit/OpenSource/Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp(304) : void WebKit::RemoteRenderingBackend::wakeUpAndApplyDisplayList(const WebKit::GPUProcessWakeupMessageArguments &) 1 0x1438c45b9 WTFCrash 2 0x117a3bf3b WTFCrashWithInfo(int, char const*, char const*, int) 3 0x1185aa181 WebKit::RemoteRenderingBackend::wakeUpAndApplyDisplayList(WebKit::GPUProcessWakeupMessageArguments const&) 4 0x1185add11 WebKit::RemoteRenderingBackend::cacheFont(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&) 5 0x11856aee0 void IPC::callMemberFunctionImpl<WebKit::RemoteRenderingBackend, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&), std::__1::tuple<WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> > >, 0ul>(WebKit::RemoteRenderingBackend*, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&), std::__1::tuple<WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> > >&&, std::__1::integer_sequence<unsigned long, 0ul>) 6 0x11856a390 void IPC::callMemberFunction<WebKit::RemoteRenderingBackend, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&), std::__1::tuple<WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> > >, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> > >&&, WebKit::RemoteRenderingBackend*, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&)) 7 0x11855debf void IPC::handleMessage<Messages::RemoteRenderingBackend::CacheFont, WebKit::RemoteRenderingBackend, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&)>(IPC::Decoder&, WebKit::RemoteRenderingBackend*, void (WebKit::RemoteRenderingBackend::*)(WTF::Ref<WebCore::Font, WTF::RawPtrTraits<WebCore::Font> >&&)) 8 0x11855d8de WebKit::RemoteRenderingBackend::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 9 0x117abdce2 IPC::Connection::dispatchMessageReceiverMessage(IPC::MessageReceiver&, std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >&&) 10 0x117ac721a IPC::WorkQueueMessageReceiverQueue::enqueueMessage(IPC::Connection&, std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >&&)::'lambda'()::operator()() 11 0x117ac6f7e WTF::Detail::CallableWrapper<IPC::WorkQueueMessageReceiverQueue::enqueueMessage(IPC::Connection&, std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >&&)::'lambda'(), void>::call() 12 0x1438ee6f2 WTF::Function<void ()>::operator()() const 13 0x143a0101e WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0::operator()() const 14 0x143a01222 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0)::'lambda'(void*)::operator()(void*) const 15 0x143a011f5 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0)::'lambda'(void*)::__invoke(void*) 16 0x7fff204c0603 _dispatch_call_block_and_release 17 0x7fff204c17e6 _dispatch_client_callout 18 0x7fff204c75ca _dispatch_lane_serial_drain 19 0x7fff204c808d _dispatch_lane_invoke 20 0x7fff204d1bed _dispatch_workloop_worker_thread 21 0x7fff206684c0 _pthread_wqthread 22 0x7fff20667493 start_wqthread LEAK: 1 WebPageProxy

I am seeing 4 new test failures from the changes in https://trac.webkit.org/changeset/279104/webkit inspector/canvas/recording-webgl-snapshots.html svg/as-image/svg-image-with-data-uri-from-canvas.html fast/canvas/canvas-path-addPath.html inspector/canvas/requestClientNodes-css.html results: https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r279119%20(3463)/results.html History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=inspector%2Fcanvas%2Frecording-webgl-snapshots.html&test=svg%2Fas-image%2Fsvg-image-with-data-uri-from-canvas.html&test=fast%2Fcanvas%2Fcanvas-path-addPath.html&test=inspector%2Fcanvas%2FrequestClientNodes-css.html

I filed bug 227277 to track in the new flatness in the canvas layout tests.

These flaky tests are causing false positive on EWS and also slowing down EWS. e.g.: False positives on EWS: https://ews-build.webkit.org/#/builders/51/builds/16145 https://ews-build.webkit.org/#/builders/51/builds/16121 https://ews-build.webkit.org/#/builders/51/builds/16117 https://ews-build.webkit.org/#/builders/51/builds/16111 https://ews-build.webkit.org/#/builders/51/builds/16101 https://ews-build.webkit.org/#/builders/51/builds/16096 Flakiness: https://ews-build.webkit.org/#/builders/51/builds/16143 https://ews-build.webkit.org/#/builders/51/builds/16132 https://ews-build.webkit.org/#/builders/51/builds/16131 https://ews-build.webkit.org/#/builders/51/builds/16119 https://ews-build.webkit.org/#/builders/51/builds/16116 https://ews-build.webkit.org/#/builders/51/builds/16112 https://ews-build.webkit.org/#/builders/51/builds/16109 https://ews-build.webkit.org/#/builders/51/builds/16103 This wastes so much of engineering time because many engineers would have to look into false positives (because of flaky failures). I am going to revert it for now. You can re-land this along-with patch from Bug 227277.

Re-opened since this is blocked by bug 227292

Created attachment 432069 [details] Patch

*** Bug 227277 has been marked as a duplicate of this bug. ***

Comment on attachment 432069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432069&action=review > Source/WebKit/ChangeLog:5 > + https://bugs.webkit.org/show_bug.cgi?id=227229 > + Radar number here please.

Created attachment 432106 [details] Patch

I talked to Myles and we agreed on fixing the RELEASE_ASSERT without removing didFinalizeRenderingUpdate(). Fixing or removing this function will addressed in a separate bug.

Created attachment 432207 [details] Patch

I filed bug 227376 to track fixing/removing RemoteResourceCacheProxy::didFinalizeRenderingUpdate().

Comment on attachment 432207 [details] Patch Kind of a shame we don't have an API test for this, that can actually kill the web process to test this codepath...

Committed r279252 (239136@main): <https://commits.webkit.org/239136@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432207 [details].