[webkit-changes] [WebKit/WebKit] 6c4c98: [WebGPU] Non-owning getters have the wrong lifetime
Myles C. Maxfield
noreply at github.com
Sat Feb 4 22:08:20 PST 2023
Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: 6c4c981002fe98d371b03ab862b589120661a63d
https://github.com/WebKit/WebKit/commit/6c4c981002fe98d371b03ab862b589120661a63d
Author: Myles C. Maxfield <mmaxfield at apple.com>
Date: 2023-02-04 (Sat, 04 Feb 2023)
Changed paths:
M Source/WebCore/PAL/PAL.xcodeproj/project.pbxproj
M Source/WebCore/PAL/pal/CMakeLists.txt
R Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceHolderImpl.cpp
R Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceHolderImpl.h
M Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceImpl.cpp
M Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceImpl.h
A Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceWrapper.cpp
A Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceWrapper.h
M Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUPresentationContextImpl.cpp
M Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUPresentationContextImpl.h
M Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUQueueImpl.cpp
M Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUQueueImpl.h
A Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainWrapper.cpp
A Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainWrapper.h
M Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUTextureImpl.cpp
M Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUTextureImpl.h
Log Message:
-----------
[WebGPU] Non-owning getters have the wrong lifetime
https://bugs.webkit.org/show_bug.cgi?id=250958
rdar://104518638
Reviewed by Dean Jackson.
There are 2 places in WebGPU where objects have getter methods that return internally-retained objects:
1. Device::getQueue() is supposed to return the same queue object every time you call it, and
2. PresentationContext::getCurrentTexture() is supposed to return the same texture object every time you call it within
the same frame.
Let's call this pattern "Owner" and "Owned." The Owner is supposed to retain its Owned. Easy peasy, right?
Well, it gets trickier because:
1. We have a corresponding set of Impl objects in PAL, each of which is supposed to maintain a strong reference to its
corresponding object in WebGPU.framework.
2. Objects exposed by WebGPU.framework are not reference counted.
So, naively, we would have:
+-----------+
| OwnerImpl |
+-----------+
| \
| \
| \
| V
| +-----------+
| | OwnedImpl |
| +-----------+
| |
~~~~~~~~|~~~~~~~~~~~~~|~~~~~~~~~~ WebGPU.framework boundary
V |
+-------+ |
| Owner | |
+-------+ |
\ |
\ |
\ |
V V BANG!!! EXPLOSION!!!
+-------+
| Owned |
+-------+
The above design can't actually work, because Owned isn't reference counted. So, instead, we can introduce a reference-
counted facade on top of Owner, to look like this:
+-----------+
| OwnerImpl |
+-----------+
| \
| \
| \
| V
| +-----------+
| | OwnedImpl |
| +-----------+
| |
| |
V V
+--------------+
| OwnerWrapper |
+--------------+
|
|
~~~~~~~~|~~~~~~~~~~~~~~~~~~~~~~~~ WebGPU.framework boundary
V
+-------+
| Owner |
+-------+
\
\
\
V
+-------+
| Owned |
+-------+
This design has all the properties we want:
1. All WebGPU.framework objects have a single owner
2. Any strong reference to the OwnerImpl keeps the Owner alive
3. Any strong reference to the OwnedImpl keeps the Owned alive
4. There are no reference cycles
The OwnedImpl doesn't actually call any functions on the OwnerWrapper; the only reason it refs it is to make the above
requirements hold.
There are 2 other possible designs which satisfy the requirements: 1. Have OwnedImpl delegate its ref() and deref()
calls to the OwnerImpl, and 2. Make WebGPU.framework objects reference counted. I chose this patch's design over (1)
because (1) is significantly more complicated and I'm more likely to make a mistake with that design. I chose this
patch's design over (2) because I didn't want to change the semantic behavior of the WebGPU.h objects.
* Source/WebCore/PAL/PAL.xcodeproj/project.pbxproj:
* Source/WebCore/PAL/pal/CMakeLists.txt:
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceImpl.cpp:
(PAL::WebGPU::DeviceImpl::DeviceImpl):
(PAL::WebGPU::DeviceImpl::destroy):
(PAL::WebGPU::DeviceImpl::createBuffer):
(PAL::WebGPU::DeviceImpl::createTexture):
(PAL::WebGPU::DeviceImpl::createSurfaceTexture):
(PAL::WebGPU::DeviceImpl::createSampler):
(PAL::WebGPU::DeviceImpl::createBindGroupLayout):
(PAL::WebGPU::DeviceImpl::createPipelineLayout):
(PAL::WebGPU::DeviceImpl::createBindGroup):
(PAL::WebGPU::DeviceImpl::createShaderModule):
(PAL::WebGPU::DeviceImpl::createComputePipeline):
(PAL::WebGPU::DeviceImpl::createRenderPipeline):
(PAL::WebGPU::DeviceImpl::createComputePipelineAsync):
(PAL::WebGPU::DeviceImpl::createRenderPipelineAsync):
(PAL::WebGPU::DeviceImpl::createCommandEncoder):
(PAL::WebGPU::DeviceImpl::createRenderBundleEncoder):
(PAL::WebGPU::DeviceImpl::createQuerySet):
(PAL::WebGPU::DeviceImpl::pushErrorScope):
(PAL::WebGPU::DeviceImpl::popErrorScope):
(PAL::WebGPU::DeviceImpl::setLabelInternal):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceImpl.h:
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceWrapper.cpp: Copied from Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceHolderImpl.cpp.
(PAL::WebGPU::DeviceWrapper::DeviceWrapper):
(PAL::WebGPU::DeviceWrapper::~DeviceWrapper):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceWrapper.h: Copied from Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceHolderImpl.h.
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUPresentationContextImpl.cpp:
(PAL::WebGPU::PresentationContextImpl::~PresentationContextImpl):
(PAL::WebGPU::PresentationContextImpl::configure):
(PAL::WebGPU::PresentationContextImpl::unconfigure):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUPresentationContextImpl.h:
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUQueueImpl.cpp:
(PAL::WebGPU::QueueImpl::QueueImpl):
(PAL::WebGPU::QueueImpl::~QueueImpl):
(PAL::WebGPU::QueueImpl::submit):
(PAL::WebGPU::QueueImpl::onSubmittedWorkDone):
(PAL::WebGPU::QueueImpl::writeBuffer):
(PAL::WebGPU::QueueImpl::writeTexture):
(PAL::WebGPU::QueueImpl::setLabelInternal):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUQueueImpl.h:
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainWrapper.cpp: Renamed from Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceHolderImpl.cpp.
(PAL::WebGPU::SwapChainWrapper::SwapChainWrapper):
(PAL::WebGPU::SwapChainWrapper::~SwapChainWrapper):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainWrapper.h: Renamed from Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceHolderImpl.h.
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUTextureImpl.cpp:
(PAL::WebGPU::TextureImpl::TextureImpl):
(PAL::WebGPU::TextureImpl::~TextureImpl):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUTextureImpl.h:
Canonical link: https://commits.webkit.org/259867@main
More information about the webkit-changes
mailing list