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.

Sign up for GitHub

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

Jump to bottom

Use CAIOSurface to cache IOSurfaces in the UI process. #11270

Merged
merged 1 commit into from Mar 12, 2023
Merged

Use CAIOSurface to cache IOSurfaces in the UI process. #11270

merged 1 commit into from Mar 12, 2023

Conversation

mattwoodrow
Copy link
Contributor

@mattwoodrow mattwoodrow commented Mar 9, 2023 •
edited by webkit-early-warning-system

e95212f

Use CAIOSurface to cache IOSurfaces in the UI process.
https://bugs.webkit.org/show_bug.cgi?id=253620

Reviewed by Simon Fraser.

We can cache CAIOSurface objects for our layer buffers in the UI process.
This keeps CA rendering data alive, without marking the IOSurface as being in-use.

* Source/WebCore/PAL/PAL.xcodeproj/project.pbxproj:
* Source/WebCore/platform/graphics/cocoa/IOSurface.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:
(WebKit::RemoteLayerBackingStore::decode):
(WebKit::RemoteLayerBackingStore::updateCachedBuffers):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeNode.h:

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

8ad069f

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

@mattwoodrow mattwoodrow requested a review from cdumez as a code owner March 9, 2023 02:49
@mattwoodrow mattwoodrow self-assigned this Mar 9, 2023
@mattwoodrow mattwoodrow added the Layout and Rendering For bugs with layout and rendering of Web pages. label Mar 9, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash f5fe53e)

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
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 9, 2023
smfr
smfr approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@smfr smfr left a comment

Choose a reason for hiding this comment

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

Please look at the layout test crash.

Source/WebCore/platform/graphics/cocoa/IOSurface.h Outdated
@@ -189,6 +191,8 @@ class IOSurface final {

RetainPtr<IOSurfaceRef> m_surface;

mutable RetainPtr<id> m_caSurface;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like a comment to say what the id really is (a CAIOSurfaceRef).

Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h Outdated
@@ -204,6 +208,11 @@ class RemoteLayerBackingStore {
// FIXME: This should be removed and m_bufferHandle should be used to ref the buffer once ShareableBitmapHandle
// can be encoded multiple times. http://webkit.org/b/234169
std::optional<MachSendRight> m_contentsBufferHandle;
RetainPtr<id> m_contentsBuffer;

std::optional<WebCore::RenderingResourceIdentifier> m_frontBufferIdentifier;
Copy link
Contributor

Choose a reason for hiding this comment

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

My review comment was going to be "Don't we have these already in Buffer classes above" but then I realize these are only used in the UI process. I think that's enough to justify either splitting RemoteLayerBackingStore into WP and UIP classes, or at least grouping the data members into structs which make it plain which process they are used it.

Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeNode.h
@@ -87,6 +87,16 @@ class RemoteLayerTreeNode : public CanMakeWeakPtr<RemoteLayerTreeNode> {
Markable<WebCore::LayerHostingContextIdentifier> remoteContextHostedIdentifier() const { return m_remoteContextHostedIdentifier; }
void setRemoteContextHostedIdentifier(WebCore::LayerHostingContextIdentifier identifier) { m_remoteContextHostedIdentifier = identifier; }

struct CachedContentsBuffer {
WebCore::RenderingResourceIdentifier m_renderingResourceIdentifier;
RetainPtr<id> m_buffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comment to say what the id really is.

@mattwoodrow mattwoodrow removed the merging-blocked Applied to prevent a change from being merged label Mar 10, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash c73c07c)

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
✅ 🛠 watch-sim

@mattwoodrow mattwoodrow added the merge-queue Applied to send a pull request to merge-queue label Mar 12, 2023
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 8ad069f. Live statuses available at the PR page, #11270

@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 8ad069f)

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

@mattwoodrow
Use CAIOSurface to cache IOSurfaces in the UI process.
e95212f 
https://bugs.webkit.org/show_bug.cgi?id=253620

Reviewed by Simon Fraser.

We can cache CAIOSurface objects for our layer buffers in the UI process.
This keeps CA rendering data alive, without marking the IOSurface as being in-use.

* Source/WebCore/PAL/PAL.xcodeproj/project.pbxproj:
* Source/WebCore/platform/graphics/cocoa/IOSurface.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:
* Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:
(WebKit::RemoteLayerBackingStore::decode):
(WebKit::RemoteLayerBackingStore::updateCachedBuffers):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeNode.h:

Canonical link: https://commits.webkit.org/261553@main
@webkit-commit-queue
Copy link
Collaborator

Committed 261553@main (e95212f): https://commits.webkit.org/261553@main

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

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

@smfr smfr smfr approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

Assignees

@mattwoodrow mattwoodrow

Labels
Layout and Rendering For bugs with layout and rendering of Web pages.
Projects
None yet
Milestone
No milestone
5 participants
@mattwoodrow @webkit-early-warning-system @webkit-commit-queue @smfr @webkit-ews-buildbot