Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GPU Process] Cache Gradient as a rendering resource #11252

Conversation

shallawa
Copy link
Contributor

@shallawa shallawa commented Mar 8, 2023

676af9d

[GPU Process] Cache Gradient as a rendering resource
https://bugs.webkit.org/show_bug.cgi?id=253599
rdar://106447608

Reviewed by Cameron McCormack.

Sending the gradient data to GPUProcess every time it is used forces creating
the platform gradient.

To avoid creating the platform gradient, we are going to cache Gradient as a
rendering resource in GPUProcess. So the object (and its platform gradient) will
stay alive in GPUProcess as long as the WebProcess gradient is alive.

* Source/WebCore/platform/graphics/DecomposedGlyphs.cpp:
(WebCore::DecomposedGlyphs::create):
(): Deleted.
* Source/WebCore/platform/graphics/DecomposedGlyphs.h:
* Source/WebCore/platform/graphics/Gradient.cpp:
(WebCore::Gradient::create):
(WebCore::Gradient::Gradient):
(WebCore::operator<<):
* Source/WebCore/platform/graphics/Gradient.h:
(WebCore::Gradient::create):
(WebCore::Gradient::colorInterpolationMethod const):
(WebCore::Gradient::spreadMethod const):
(WebCore::Gradient::stops const):
* Source/WebCore/platform/graphics/GraphicsContextState.h:
(WebCore::GraphicsContextState::fillBrush):
(WebCore::GraphicsContextState::strokeBrush):
* Source/WebCore/platform/graphics/NativeImage.h:
* Source/WebCore/platform/graphics/RenderingResource.h:
(WebCore::RenderingResource::~RenderingResource):
(WebCore::RenderingResource::hasValidRenderingResourceIdentifier const):
(WebCore::RenderingResource::renderingResourceIdentifier const):
(WebCore::RenderingResource::addObserver):
(WebCore::RenderingResource::removeObserver):
(WebCore::RenderingResource::RenderingResource):
* Source/WebCore/platform/graphics/SourceBrush.cpp:
(WebCore::SourceBrush::gradientSpaceTransform const):
(WebCore::SourceBrush::gradient const):
(WebCore::SourceBrush::gradientIdentifier const):
(WebCore::SourceBrush::setGradient):
* Source/WebCore/platform/graphics/SourceBrush.h:
(WebCore::SourceBrush::setGradient):
(WebCore::operator==):
(WebCore::SourceBrush::Brush::LogicalGradient::encode const):
(WebCore::SourceBrush::Brush::LogicalGradient::decode):
* Source/WebCore/platform/graphics/displaylists/DisplayList.h:
(WebCore::DisplayList::DisplayList::cacheDecomposedGlyphs):
(WebCore::DisplayList::DisplayList::cacheGradient):
* Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:
(WebCore::DisplayList::SetState::state):
* Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:
(WebCore::DisplayList::Recorder::appendStateChangeItem):
* Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:
* Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.cpp:
(WebCore::DisplayList::RecorderImpl::recordResourceUse):
* Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.h:
* Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:
(WebCore::DisplayList::LocalResourceHeap::add):
* Source/WebCore/rendering/svg/RenderSVGResourceLinearGradient.cpp:
(WebCore::RenderSVGResourceLinearGradient::buildGradient const):
* Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:
(WebCore::RenderSVGResourceRadialGradient::buildGradient const):
* Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:
(WebKit::QualifiedResourceHeap::add):
(WebKit::QualifiedResourceHeap::getGradient const):
(WebKit::QualifiedResourceHeap::removeGradient):
(WebKit::QualifiedResourceHeap::releaseAllResources):
(WebKit::QualifiedResourceHeap::checkInvariants const):
* Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp:
(WebKit::RemoteDisplayListRecorder::setState):
* Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:
(WebKit::RemoteRenderingBackend::cacheGradient):
(WebKit::RemoteRenderingBackend::cacheGradientWithQualifiedIdentifier):
* Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h:
* Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in:
* Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:
(WebKit::RemoteResourceCache::cacheGradient):
(WebKit::RemoteResourceCache::cachedGradient const):
(WebKit::RemoteResourceCache::releaseRenderingResource):
* Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:
* Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in:
* Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp:
(WebKit::RemoteDisplayListRecorderProxy::recordResourceUse):
* Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h:
* Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:
(WebKit::RemoteRenderingBackendProxy::cacheGradient):
* Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h:
* Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:
(WebKit::RemoteResourceCacheProxy::~RemoteResourceCacheProxy):
(WebKit::RemoteResourceCacheProxy::clear):
(WebKit::RemoteResourceCacheProxy::recordGradientUse):
(WebKit::RemoteResourceCacheProxy::releaseRenderingResource):
(WebKit::RemoteResourceCacheProxy::clearGradientMap):
(WebKit::RemoteResourceCacheProxy::remoteResourceCacheWasDestroyed):
(WebKit::RemoteResourceCacheProxy::releaseMemory):
* Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:

Canonical link: https://commits.webkit.org/262185@main

78a0467

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2 ⏳ πŸ§ͺ api-gtk
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim

@shallawa shallawa requested a review from cdumez as a code owner March 8, 2023 21:31
@shallawa shallawa self-assigned this Mar 8, 2023
@shallawa shallawa added the Layout and Rendering For bugs with layout and rendering of Web pages. label Mar 8, 2023
@shallawa shallawa force-pushed the eng/GPU-Process-Cache-Gradient-as-a-rendering-resource branch from 2b39fb0 to dac1862 Compare March 9, 2023 23:48
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 10, 2023
@shallawa shallawa removed the merging-blocked Applied to prevent a change from being merged label Mar 16, 2023
@shallawa shallawa force-pushed the eng/GPU-Process-Cache-Gradient-as-a-rendering-resource branch from dac1862 to 9f44c23 Compare March 16, 2023 22:25
@shallawa shallawa requested review from smfr and heycam March 20, 2023 17:42
@shallawa shallawa force-pushed the eng/GPU-Process-Cache-Gradient-as-a-rendering-resource branch from 9f44c23 to 78a0467 Compare March 24, 2023 00:43
Comment on lines +53 to +56
bool hasValidRenderingResourceIdentifier() const
{
return m_renderingResourceIdentifier.has_value();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we create resources without a valid RenderingResourceIdentifier? Is it that we don't provide an identifier for gradients that are cheap to send across or something? If so, instead of doing it based on whether the identifier is valid, is it something the encode/decode functions can decide by inspecting the gradient? Then we can continue to have valid identifiers always.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When trying to cache all gradients, I noticed MotionMark CanvasLines regressed significantly. The reason for this is CanvasLines creates a gradient every time a line is drawn https://github.com/WebKit/WebKit/blob/main/PerformanceTests/MotionMark/tests/master/resources/canvas-tests.js#L238. So caching this gradient was not beneficial at all since each gradient is just used only once. This contrary to SVG gradients which lives as long as the SVG object lives. So only SVG gradients will have valid RenderingResourceIdentifier.

@shallawa shallawa added the merge-queue Applied to send a pull request to merge-queue label Mar 27, 2023
https://bugs.webkit.org/show_bug.cgi?id=253599
rdar://106447608

Reviewed by Cameron McCormack.

Sending the gradient data to GPUProcess every time it is used forces creating
the platform gradient.

To avoid creating the platform gradient, we are going to cache Gradient as a
rendering resource in GPUProcess. So the object (and its platform gradient) will
stay alive in GPUProcess as long as the WebProcess gradient is alive.

* Source/WebCore/platform/graphics/DecomposedGlyphs.cpp:
(WebCore::DecomposedGlyphs::create):
(): Deleted.
* Source/WebCore/platform/graphics/DecomposedGlyphs.h:
* Source/WebCore/platform/graphics/Gradient.cpp:
(WebCore::Gradient::create):
(WebCore::Gradient::Gradient):
(WebCore::operator<<):
* Source/WebCore/platform/graphics/Gradient.h:
(WebCore::Gradient::create):
(WebCore::Gradient::colorInterpolationMethod const):
(WebCore::Gradient::spreadMethod const):
(WebCore::Gradient::stops const):
* Source/WebCore/platform/graphics/GraphicsContextState.h:
(WebCore::GraphicsContextState::fillBrush):
(WebCore::GraphicsContextState::strokeBrush):
* Source/WebCore/platform/graphics/NativeImage.h:
* Source/WebCore/platform/graphics/RenderingResource.h:
(WebCore::RenderingResource::~RenderingResource):
(WebCore::RenderingResource::hasValidRenderingResourceIdentifier const):
(WebCore::RenderingResource::renderingResourceIdentifier const):
(WebCore::RenderingResource::addObserver):
(WebCore::RenderingResource::removeObserver):
(WebCore::RenderingResource::RenderingResource):
* Source/WebCore/platform/graphics/SourceBrush.cpp:
(WebCore::SourceBrush::gradientSpaceTransform const):
(WebCore::SourceBrush::gradient const):
(WebCore::SourceBrush::gradientIdentifier const):
(WebCore::SourceBrush::setGradient):
* Source/WebCore/platform/graphics/SourceBrush.h:
(WebCore::SourceBrush::setGradient):
(WebCore::operator==):
(WebCore::SourceBrush::Brush::LogicalGradient::encode const):
(WebCore::SourceBrush::Brush::LogicalGradient::decode):
* Source/WebCore/platform/graphics/displaylists/DisplayList.h:
(WebCore::DisplayList::DisplayList::cacheDecomposedGlyphs):
(WebCore::DisplayList::DisplayList::cacheGradient):
* Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:
(WebCore::DisplayList::SetState::state):
* Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:
(WebCore::DisplayList::Recorder::appendStateChangeItem):
* Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:
* Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.cpp:
(WebCore::DisplayList::RecorderImpl::recordResourceUse):
* Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.h:
* Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:
(WebCore::DisplayList::LocalResourceHeap::add):
* Source/WebCore/rendering/svg/RenderSVGResourceLinearGradient.cpp:
(WebCore::RenderSVGResourceLinearGradient::buildGradient const):
* Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:
(WebCore::RenderSVGResourceRadialGradient::buildGradient const):
* Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:
(WebKit::QualifiedResourceHeap::add):
(WebKit::QualifiedResourceHeap::getGradient const):
(WebKit::QualifiedResourceHeap::removeGradient):
(WebKit::QualifiedResourceHeap::releaseAllResources):
(WebKit::QualifiedResourceHeap::checkInvariants const):
* Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp:
(WebKit::RemoteDisplayListRecorder::setState):
* Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:
(WebKit::RemoteRenderingBackend::cacheGradient):
(WebKit::RemoteRenderingBackend::cacheGradientWithQualifiedIdentifier):
* Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h:
* Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in:
* Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:
(WebKit::RemoteResourceCache::cacheGradient):
(WebKit::RemoteResourceCache::cachedGradient const):
(WebKit::RemoteResourceCache::releaseRenderingResource):
* Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:
* Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in:
* Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp:
(WebKit::RemoteDisplayListRecorderProxy::recordResourceUse):
* Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.h:
* Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:
(WebKit::RemoteRenderingBackendProxy::cacheGradient):
* Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h:
* Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:
(WebKit::RemoteResourceCacheProxy::~RemoteResourceCacheProxy):
(WebKit::RemoteResourceCacheProxy::clear):
(WebKit::RemoteResourceCacheProxy::recordGradientUse):
(WebKit::RemoteResourceCacheProxy::releaseRenderingResource):
(WebKit::RemoteResourceCacheProxy::clearGradientMap):
(WebKit::RemoteResourceCacheProxy::remoteResourceCacheWasDestroyed):
(WebKit::RemoteResourceCacheProxy::releaseMemory):
* Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:

Canonical link: https://commits.webkit.org/262185@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/GPU-Process-Cache-Gradient-as-a-rendering-resource branch from 78a0467 to 676af9d Compare March 28, 2023 00:14
@webkit-commit-queue
Copy link
Collaborator

Committed 262185@main (676af9d): https://commits.webkit.org/262185@main

Reviewed commits have been landed. Closing PR #11252 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 28, 2023
@webkit-commit-queue webkit-commit-queue merged commit 676af9d into WebKit:main Mar 28, 2023
@philn
Copy link
Member

philn commented Mar 28, 2023

Thiis broke linux clang builds...

In file included from /app/webkit/WebKitBuild/Release/WebCore/DerivedSources/unified-sources/UnifiedSource-3c72abbe-35.cpp:1:                                                                                        
In file included from /app/webkit/Source/WebCore/platform/graphics/RenderingMode.cpp:29:                                                                                                                             
In file included from /app/webkit/WebKitBuild/Release/WTF/Headers/wtf/text/TextStream.h:29:                                                                                                                          
In file included from /app/webkit/WebKitBuild/Release/WTF/Headers/wtf/Markable.h:39:                                                                                                                                 
In file included from /app/webkit/WebKitBuild/Release/WTF/Headers/wtf/Hasher.h:27:                                                                                                                                   
In file included from /app/webkit/WebKitBuild/Release/WTF/Headers/wtf/URL.h:28:                                                                                                                                      
In file included from /app/webkit/WebKitBuild/Release/WTF/Headers/wtf/text/WTFString.h:28:                                                                                                                           
In file included from /app/webkit/WebKitBuild/Release/WTF/Headers/wtf/text/StringImpl.h:29:                                                                                                                          
In file included from /app/webkit/WebKitBuild/Release/WTF/Headers/wtf/CompactPtr.h:32:                                                                                                                               
In file included from /app/webkit/WebKitBuild/Release/WTF/Headers/wtf/HashFunctions.h:26:                                                                                                                            
In file included from /app/webkit/WebKitBuild/Release/WTF/Headers/wtf/RefPtr.h:28:                                                                                                                                   
/app/webkit/WebKitBuild/Release/WTF/Headers/wtf/Ref.h:90:11: error: cannot initialize a member subobject of type 'typename PtrTraits::StorageType' (aka 'WebCore::Gradient *') with an rvalue of type 'WebCore::Patte
rn *'                                                                                                                                                                                                                
        : m_ptr(&other.leakRef())                                                                                                                                                                                    
          ^     ~~~~~~~~~~~~~~~~                                                                                                                                                                                     
/usr/lib/gcc/x86_64-unknown-linux-gnu/12.2.0/../../../../include/c++/12.2.0/variant:284:6: note: in instantiation of function template specialization 'WTF::Ref<WebCore::Gradient>::Ref<WebCore::Pattern, WTF::RawPtr
Traits<WebCore::Pattern>>' requested here                                                                                                                                                                            
     _Type(std::forward<_Args>(__args)...);                                                                                                                                                                          
     ^                                                                                                                                                                                                               
1 error generated.                                                                   

Would it be possible to simplify the struct Brush layout to avoid embedding variants in variants please? The current setup seems ambiguous, somehow...

@shallawa
Copy link
Contributor Author

Would it be possible to simplify the struct Brush layout to avoid embedding variants in variants please? The current setup seems ambiguous, somehow...

struct Brush can be either a LogicalGradient or Ref<Pattern>. This is what GraphicsContextState wants to fill or stroke with.

If it is a LogicalGradient it will have either a Ref<Gradient> or RenderingResourceIdentifier besides the spaceTransform. Representing the LogicalGradient with RenderingResourceIdentifier is used while encoding the Brush.

So why do think this structure is not simplified and seems ambiguous?

I could not understand from the lines above which line in the code causes this error. Can you please include more lines from the compilation output?

@philn
Copy link
Member

philn commented Mar 28, 2023

That's the beauty of it, thanks to unified builds, the error message is almost useless. What I shared is what's reported by the tooling, I don't have more details...

@zdobersek mentioned in-place constructors might help

@shallawa
Copy link
Contributor Author

That's the beauty of it, thanks to unified builds, the error message is almost useless. What I shared is what's reported by the tooling, I don't have more details...

I think there is a build option to disable local unified builds. @JonWBedard might know more about this option.

@zdobersek
Copy link
Contributor

Fixed in #12177.

@shallawa shallawa deleted the eng/GPU-Process-Cache-Gradient-as-a-rendering-resource branch May 9, 2023 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Layout and Rendering For bugs with layout and rendering of Web pages.
Projects
None yet
7 participants