Skip to content

Conversation

@donny-dont
Copy link
Contributor

@donny-dont donny-dont commented Apr 20, 2023

a6579e4

Use move semantics for SharedMemory::Handle
https://bugs.webkit.org/show_bug.cgi?id=255745

Reviewed by Kimmo Kinnunen.

Use move semantics to signify ownership of the `SharedMemory::Handle`.

* Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp:
* Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.h:
* Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.h:
* Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLCocoa.cpp:
* Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:
* Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.cpp:
* Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.h:
* Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.h:
* Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.mm:
* Source/WebKit/GPUProcess/webrtc/RemoteMediaRecorder.cpp:
* Source/WebKit/GPUProcess/webrtc/RemoteMediaRecorder.h:
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.cpp:
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.h:
* Source/WebKit/Platform/IPC/SharedBufferReference.cpp:
* Source/WebKit/Platform/IPC/StreamServerConnectionBuffer.h:
* Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:
* Source/WebKit/Platform/SharedMemory.h:
* Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:
* Source/WebKit/Platform/unix/SharedMemoryUnix.cpp:
* Source/WebKit/Platform/win/SharedMemoryWin.cpp:
* Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:
* Source/WebKit/Shared/ShareableBitmap.cpp:
* Source/WebKit/Shared/ShareableResource.cpp:
* Source/WebKit/Shared/WebCompiledContentRuleListData.cpp:
* Source/WebKit/Shared/WebCoreArgumentCoders.cpp:
* Source/WebKit/Shared/WebHitTestResultData.cpp:
* Source/WebKit/Shared/WebHitTestResultData.h:
* Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:
* Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:
* Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp:
* Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.h:
* Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayer.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.h:
* Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:
* Source/WebKit/WebProcess/Storage/WebSWClientConnection.h:
* Source/WebKit/WebProcess/Storage/WebSWOriginTable.cpp:
* Source/WebKit/WebProcess/Storage/WebSWOriginTable.h:
* Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:
* Source/WebKit/WebProcess/WebPage/VisitedLinkTableController.cpp:
* Source/WebKit/WebProcess/WebPage/VisitedLinkTableController.h:
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
* Source/WebKit/WebProcess/WebPage/WebPage.h:

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

1b27815

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@donny-dont donny-dont requested review from a team and cdumez as code owners April 20, 2023 22:52
@donny-dont donny-dont self-assigned this Apr 20, 2023
@donny-dont donny-dont added the Platform Portability improvements and other general platform improvements not driven directly by site bugs. label Apr 20, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 20, 2023
@donny-dont donny-dont removed the merging-blocked Applied to prevent a change from being merged label Apr 20, 2023
@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from 95eab95 to 07f99b2 Compare April 20, 2023 23:11
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't SharedMemory::Handle essentially just an integer (or 2 with the size)? If so, it doesn't seem like there would be much benefit to moving it around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This patch started when I asked @kkinnunen-apple about what the behavior of SharedMemory::map was supposed to be with regards to the handle passed in. The behavior was not consistent between the different backend implementations, Win, Unix, Cocoa. I found this out when enabling GPU Process on PlayStation where the Unix variant of SharedMemory::createHandle asserted but the Windows one was fine.

So @kkinnunen-apple expressed a plan to refactor SharedMemory::map to take a Handle&& but wasn't available to work on it till later. Since I was broken I took a stab at this. I THINK I replicated their plan but I'll let them chime in.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 This is more about transferring ownership when optimizing copies. Seems fine.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 20, 2023
@donny-dont donny-dont removed the merging-blocked Applied to prevent a change from being merged label Apr 20, 2023
@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from 07f99b2 to e098478 Compare April 20, 2023 23:42
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 20, 2023
Copy link
Contributor

@kkinnunen-apple kkinnunen-apple left a comment

Choose a reason for hiding this comment

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

Great start! Still a bit of work to do, though! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to remove the "mutable" from " m_handle;" declarations above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this indicates a contagiousness of the change:
All the types that contain SharedMemoryHandle must now support move semantics also.
So before on Cocoa, SharedMemory::map did not mutate the handle. This meant that the ShareableBitmapHandle handle here would stay intact and operational after ShareableBitmap::create(). Now, after your change, the m_handle is moved away and the ShareableBitmapHandle is rendered inoperable.
It is unknown if any code relied on the ShareableBitmapHandle staying operable.

Thus to prove correctness, the approach would be to first make ShareableBitmapHandle movable, land that, and then continue with this patch.

RefPtr<ShareableBitmap> ShareableBitmap::create(const ShareableBitmapHandle& handle, SharedMemory::Protection protection)
->
RefPtr<ShareableBitmap> ShareableBitmap::create(ShareableBitmapHandle&& handle, SharedMemory::Protection protection)

Copy link
Contributor

Choose a reason for hiding this comment

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

This lacks WTFMove (the compiler will tell you this, though)

Copy link
Contributor

@kkinnunen-apple kkinnunen-apple Apr 21, 2023

Choose a reason for hiding this comment

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

So here StreamServerConnectionBuffer::Handle is a function where the author already thought that handles have to have move semantics as the consumption operation, and thus already declared Handle as && so you don't need to do the same thing as with ShareableBitmap/ShareableResource. 😉

@donny-dont donny-dont removed the merging-blocked Applied to prevent a change from being merged label Apr 25, 2023
@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from e098478 to d953dee Compare April 25, 2023 21:23
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 25, 2023
@donny-dont donny-dont removed the merging-blocked Applied to prevent a change from being merged label Apr 25, 2023
@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from d953dee to ab14f05 Compare April 25, 2023 22:03
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 25, 2023
@donny-dont donny-dont removed the merging-blocked Applied to prevent a change from being merged label Apr 25, 2023
@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from ab14f05 to aada203 Compare April 25, 2023 22:26
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 25, 2023
@donny-dont donny-dont removed the merging-blocked Applied to prevent a change from being merged label May 4, 2023
@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from aada203 to c52ac16 Compare May 4, 2023 20:34
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 4, 2023
@donny-dont donny-dont removed the merging-blocked Applied to prevent a change from being merged label May 4, 2023
@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from c52ac16 to b987200 Compare May 4, 2023 21:43
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 4, 2023
@donny-dont donny-dont removed the merging-blocked Applied to prevent a change from being merged label May 4, 2023
@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from b987200 to 9f11c7c Compare May 4, 2023 22:03
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 4, 2023
@donny-dont donny-dont removed the merging-blocked Applied to prevent a change from being merged label May 5, 2023
@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from 9f11c7c to 0af1cec Compare May 5, 2023 00:26
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 5, 2023

@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from 0af1cec to 85b409b Compare May 5, 2023 20:17
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 5, 2023

@donny-dont donny-dont force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from 85b409b to 1b27815 Compare May 8, 2023 04:54
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 8, 2023

@donny-dont donny-dont added the merge-queue Applied to send a pull request to merge-queue label May 8, 2023
https://bugs.webkit.org/show_bug.cgi?id=255745

Reviewed by Kimmo Kinnunen.

Use move semantics to signify ownership of the `SharedMemory::Handle`.

* Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp:
* Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.h:
* Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.h:
* Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLCocoa.cpp:
* Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:
* Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.cpp:
* Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.h:
* Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.h:
* Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.mm:
* Source/WebKit/GPUProcess/webrtc/RemoteMediaRecorder.cpp:
* Source/WebKit/GPUProcess/webrtc/RemoteMediaRecorder.h:
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.cpp:
* Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayer.h:
* Source/WebKit/Platform/IPC/SharedBufferReference.cpp:
* Source/WebKit/Platform/IPC/StreamServerConnectionBuffer.h:
* Source/WebKit/Platform/IPC/unix/ConnectionUnix.cpp:
* Source/WebKit/Platform/SharedMemory.h:
* Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:
* Source/WebKit/Platform/unix/SharedMemoryUnix.cpp:
* Source/WebKit/Platform/win/SharedMemoryWin.cpp:
* Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:
* Source/WebKit/Shared/ShareableBitmap.cpp:
* Source/WebKit/Shared/ShareableResource.cpp:
* Source/WebKit/Shared/WebCompiledContentRuleListData.cpp:
* Source/WebKit/Shared/WebCoreArgumentCoders.cpp:
* Source/WebKit/Shared/WebHitTestResultData.cpp:
* Source/WebKit/Shared/WebHitTestResultData.h:
* Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:
* Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:
* Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp:
* Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/RemoteVideoFrameObjectHeapProxyProcessor.h:
* Source/WebKit/WebProcess/GPU/webrtc/SampleBufferDisplayLayer.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.cpp:
* Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.h:
* Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:
* Source/WebKit/WebProcess/Storage/WebSWClientConnection.h:
* Source/WebKit/WebProcess/Storage/WebSWOriginTable.cpp:
* Source/WebKit/WebProcess/Storage/WebSWOriginTable.h:
* Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:
* Source/WebKit/WebProcess/WebPage/VisitedLinkTableController.cpp:
* Source/WebKit/WebProcess/WebPage/VisitedLinkTableController.h:
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
* Source/WebKit/WebProcess/WebPage/WebPage.h:

Canonical link: https://commits.webkit.org/263809@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Use-move-semantics-for-SharedMemoryHandle branch from 1b27815 to a6579e4 Compare May 8, 2023 18:34
@webkit-commit-queue
Copy link
Collaborator

Committed 263809@main (a6579e4): https://commits.webkit.org/263809@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 8, 2023
@webkit-commit-queue webkit-commit-queue merged commit a6579e4 into WebKit:main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Platform Portability improvements and other general platform improvements not driven directly by site bugs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants