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
[WebGPU] MTLResource allocations should be attributed to the respective web process #11787
Conversation
EWS run on previous version of this PR (hash c73c13e) |
c73c13e
to
19866b4
Compare
EWS run on previous version of this PR (hash 19866b4) |
Source/WebGPU/WebGPU/Device.mm
Outdated
@@ -189,6 +198,24 @@ static void captureFrame(id<MTLDevice> captureObject) | |||
m_isLost = true; | |||
} | |||
|
|||
static void setOwnerWithIdentity(id<MTLResourceSPI> resource, auto webProcessID) | |||
{ | |||
ASSERT([resource respondsToSelector:@selector(setOwnerWithIdentity:)]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delegate method is optional hence the respondsToSelector
check. But from what I can tell all resources we create implement it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can avoid the double respondsToSelector by structuring the statement as WebKit-style early return + ASSERT_NOT_REACHED()
19866b4
to
a47903d
Compare
@@ -96,7 +96,7 @@ class RemoteGPUProxy final : public PAL::WebGPU::GPU, private IPC::Connection::C | |||
} | |||
IPC::Connection& connection() const { return m_gpuProcessConnection->connection(); } | |||
|
|||
void requestAdapter(const PAL::WebGPU::RequestAdapterOptions&, CompletionHandler<void(RefPtr<PAL::WebGPU::Adapter>&&)>&&) final; | |||
void requestAdapter(const PAL::WebGPU::RequestAdapterOptions&, CompletionHandler<void(RefPtr<PAL::WebGPU::Adapter>&&)>&&, unsigned = 0) final; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the meaning of this? It needs a name. (And it seems unused in the .cpp file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this up
@@ -118,7 +118,7 @@ void RemoteGPUProxy::waitUntilInitialized() | |||
m_lost = true; | |||
} | |||
|
|||
void RemoteGPUProxy::requestAdapter(const PAL::WebGPU::RequestAdapterOptions& options, CompletionHandler<void(RefPtr<PAL::WebGPU::Adapter>&&)>&& callback) | |||
void RemoteGPUProxy::requestAdapter(const PAL::WebGPU::RequestAdapterOptions& options, CompletionHandler<void(RefPtr<PAL::WebGPU::Adapter>&&)>&& callback, unsigned) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem used?
This indicates a leaky abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to factor this away to avoid this issue
auto remoteCompositorIntegration = RemoteCompositorIntegration::create(compositorIntegration, m_objectHeap, *m_streamConnection, identifier); | ||
m_objectHeap->addObject(identifier, remoteCompositorIntegration); | ||
} | ||
|
||
unsigned RemoteGPU::webProcessID() const | ||
{ | ||
return m_gpuConnectionToWebProcess->webProcessIdentity().taskIdToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use the identity throughout our code, and only call taskIdToken()
at the latest possible moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProcessIdentity
is in WebCore/platform
so unfortunately I think this is the latest possible moment.
We can also forward declare ProcessIdentity
and pass it throughout in PAL but then where do we use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:/ ok
@@ -42,7 +42,7 @@ class GPU : public RefCounted<GPU> { | |||
public: | |||
virtual ~GPU() = default; | |||
|
|||
virtual void requestAdapter(const RequestAdapterOptions&, CompletionHandler<void(RefPtr<Adapter>&&)>&&) = 0; | |||
virtual void requestAdapter(const RequestAdapterOptions&, CompletionHandler<void(RefPtr<Adapter>&&)>&&, unsigned = 0) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a better way to do this? Every GPU
in the GPU process will come from a different web process, can't this information be in the GPU
object (and not passed to requestAdapter())?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could store it in the GPU object instead. This would require from what I can tell storing the GPU object in the Adapter or the Device so we can get the web process id.
Do you think storing a GPU& in the adapter is a good idea? I can do that if so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's the design we have already - child objects hold strong references to their parent object
EWS run on previous version of this PR (hash a47903d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It generally seems good, but I think we should rejigger the handshake between the GPU and the Device, so that we don't have to modify pal/graphics/WebGPU/WebGPU.h
.
@@ -44,6 +44,7 @@ class CompositorIntegration : public RefCounted<CompositorIntegration> { | |||
|
|||
#if PLATFORM(COCOA) | |||
virtual Vector<MachSendRight> recreateRenderBuffers(int width, int height) = 0; | |||
virtual void setWebProcessID(unsigned) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is a perfect example of why the Compositor Integration object exists - it's our own object with our own API that we fully control.
Source/WebGPU/WebGPU/Adapter.h
Outdated
@@ -64,16 +64,17 @@ class Adapter : public WGPUAdapterImpl, public RefCounted<Adapter> { | |||
void makeInvalid() { m_device = nil; } | |||
|
|||
Instance& instance() const { return m_instance; } | |||
|
|||
unsigned webProcessID() const { return m_webProcessID; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this belongs here :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh maybe it should go in Instance
? The name of that had me thinking there was only one Instance
instance per GPU process. But it seems there is one Instance
instance in the GPU process for each web process?
Source/WebGPU/WebGPU/Device.mm
Outdated
#if USE(INTERNAL_APPLE_SDK) | ||
#import <Metal/MTLResource_Private.h> | ||
#else | ||
@protocol MTLResourceSPI <MTLResource> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we put these in their own MetalSPI.h file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we introduce a new MetalSPI.h for WebGPU.framework? I don't think one exists yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add one since I don't think a single SPI usage warrants its own file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should make a MetalSPI.h. It's important we keep accounting of SPI clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SPI header added :)
return [m_device newBufferWithLength:std::max(static_cast<NSUInteger>(1), length) options:resourceOptions]; | ||
id<MTLBuffer> buffer = [m_device newBufferWithLength:std::max(static_cast<NSUInteger>(1), length) options:resourceOptions]; | ||
ASSERT(buffer); | ||
setOwnerWithIdentity(buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand, the setOwnerWithIdentity()
function is on Device
but I don't see a m_device.
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are inside Device::safeCreateBuffer
Source/WebGPU/WebGPU/Device.mm
Outdated
id<MTLBuffer> Device::newBufferWithBytes(const void *pointer, NSUInteger length, MTLResourceOptions options) const | ||
{ | ||
id<MTLBuffer> buffer = [m_device newBufferWithBytes:pointer length:length options:options]; | ||
ASSERT(buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really right to ASSERT() success? Don't we need to handle the case where allocation failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we should handle that case. I'll file a bug to track that. The debug assert is to catch the failure when it happens as opposed to letting it go unnoticed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm switching to approve because I think it's generally good, but I still think we should rejigger the handshake.
Source/WebGPU/WebGPU/Device.mm
Outdated
void Device::setOwnerWithIdentity(id<MTLResource> resource) const | ||
{ | ||
auto webProcessID = m_adapter->webProcessID(); | ||
if (!webProcessID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this deserves a comment saying "WebKit1" or "In-process WebGPU" or something
#endif | ||
|
||
WTF::Function<void(CompletionHandler<void()>&&)> m_onSubmittedWorkScheduledCallback; | ||
|
||
RefPtr<PresentationContextImpl> m_presentationContext; | ||
Ref<ConvertToBackingContext> m_convertToBackingContext; | ||
unsigned m_webProcessID { 0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a mach_foo_t
type we can use instead of unsigned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is mach_port_t
which is typedef as unsigned int
but I don't think that is available in PAL. I could wrap all the usage of it in #ifdef
however but this defeats the purpose of having a PAL
I'm going to see if I can refactor some of these changes to have minimal impact on PAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched to mach_port_t
as most PAL changes were removed. I think it will be fine, if there is a build issue on some platform, I will add a typedef
a47903d
to
83eaccb
Compare
@litherum it's much simpler now :) in case you want to take a second look |
EWS run on previous version of this PR (hash 83eaccb) |
83eaccb
to
e3cd683
Compare
EWS run on previous version of this PR (hash e3cd683) |
7f52e17
to
db34b15
Compare
EWS run on previous version of this PR (hash db34b15) |
db34b15
to
11a3d18
Compare
EWS run on previous version of this PR (hash 11a3d18) |
11a3d18
to
d117ecf
Compare
EWS run on previous version of this PR (hash d117ecf) |
Failed api-ios checks. Please resolve failures and re-apply Rejecting #11787 from merge queue. |
Safe-Merge-Queue: Build #15310. |
d117ecf
to
9d70059
Compare
EWS run on current version of this PR (hash 9d70059) |
β¦ve web process https://bugs.webkit.org/show_bug.cgi?id=254219 <radar://107001772> Reviewed by Tadeu Zagallo and Myles C. Maxfield. Attribute memory used by IOSurfaces and MTLResource instances to the corresponding web process, so we do not exceed jetsam limits on iOS. Mostly this involves piping the web process ID which already is accessible via RemoteGPU down to WebGPU.framework and then calling the appropriate IOSurface and Metal SPIs. * Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUCreateImpl.cpp: (PAL::WebGPU::create): * Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUCreateImpl.h: * Source/WebGPU/WebGPU/Buffer.mm: (WebGPU::Device::safeCreateBuffer const): * Source/WebGPU/WebGPU/Device.h: * Source/WebGPU/WebGPU/Device.mm: (WebGPU::setOwnerWithIdentity): (WebGPU::Device::setOwnerWithIdentity const): (WebGPU::Device::newBufferWithBytes const): (WebGPU::Device::newTextureWithDescriptor const): * Source/WebGPU/WebGPU/Instance.h: * Source/WebGPU/WebGPU/Instance.mm: (WebGPU::Instance::create): (WebGPU::Instance::Instance): (WebGPU::m_webProcessID): (WebGPU::Instance::createSurface): (WebGPU::Instance::webProcessID const): * Source/WebGPU/WebGPU/PresentationContext.h: * Source/WebGPU/WebGPU/PresentationContext.mm: (WebGPU::PresentationContext::create): * Source/WebGPU/WebGPU/PresentationContextIOSurface.h: * Source/WebGPU/WebGPU/PresentationContextIOSurface.mm: (WebGPU::PresentationContextIOSurface::create): (WebGPU::PresentationContextIOSurface::PresentationContextIOSurface): (WebGPU::PresentationContextIOSurface::renderBuffersWereRecreated): (WebGPU::PresentationContextIOSurface::configure): * Source/WebGPU/WebGPU/Queue.mm: (WebGPU::Queue::writeBuffer): (WebGPU::Queue::writeTexture): * Source/WebGPU/WebGPU/Texture.mm: (WebGPU::Device::createTexture): * Source/WebGPU/WebGPU/WebGPUExt.h: * Source/WebKit/GPUProcess/graphics/WebGPU/RemoteGPU.cpp: (WebKit::webProcessID): (WebKit::RemoteGPU::workQueueInitialize): * Source/WebGPU/WebGPU/MetalSPI.h: Add SPI header. Canonical link: https://commits.webkit.org/276332@main
9d70059
to
d984488
Compare
Committed 276332@main (d984488): https://commits.webkit.org/276332@main Reviewed commits have been landed. Closing PR #11787 and removing active labels. |
d984488
9d70059
π§ͺ wpe-wk2π§ͺ api-gtk