[Cocoa] Temporarily wake up remote process if outgoing IPC message queue becomes too large #13340

cdumez · 2023-05-01T21:58:02Z

`5183343`

[Cocoa] Temporarily wake up remote process if outgoing IPC message queue becomes too large
https://bugs.webkit.org/show_bug.cgi?id=256181

Reviewed by Ben Nham.

Temporarily wake up remote process if outgoing IPC message queue becomes too
large, to avoid potentially bad memory growth when the remote process is
suspended.

This currently only works for AuxiliaryProcessProxy, so IPC from the UIProcess
to child processes. However, we may want to consider extending this to IPC
from the network process to WebProcesses in a follow-up.

In the current proposal, we wake up the destination process for 3 seconds when
its outgoing message queue has reached 1024 messages. We do so at most once
every 30 seconds.

* Source/WebKit/Platform/IPC/Connection.cpp:
(IPC::Connection::setOutgoingMessageQueueIsGrowingLargeCallback):
(IPC::Connection::invalidate):
(IPC::Connection::sendMessage):
* Source/WebKit/Platform/IPC/Connection.h:
* Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:
(WebKit::AuxiliaryProcessProxy::AuxiliaryProcessProxy):
(WebKit::AuxiliaryProcessProxy::didFinishLaunching):
(WebKit::AuxiliaryProcessProxy::outgoingMessageQueueIsGrowingLarge):
* Source/WebKit/UIProcess/AuxiliaryProcessProxy.h:

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

966aea0

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

webkit-early-warning-system · 2023-05-01T21:58:11Z

EWS run on previous version of this PR (hash 17a41bd)

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~~
⏳ 🛠 🧪 jsc	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
⏳ 🛠 🧪 jsc-arm64	⏳ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	⏳ 🛠 tv-sim
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-05-01T21:59:50Z

EWS run on previous version of this PR (hash 9e5d96d)

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

bnham · 2023-05-01T22:28:09Z

Source/WebKit/Platform/IPC/Connection.cpp

 #endif

+constexpr size_t largeOutgoingMessageQueueCountThreshold { 500 };


I'd suggest picking 1024 to match MACH_PORT_QLIMIT_LARGE. So basically in the worst case this will trigger after ~2048 messages if the receiver process is suspended (1024 will be stuck in the Mach IPC queue and 1024 will be stuck in m_outgoingMessages.

I also realize this only applies to UIProcess at the moment, but when running some LocalStorage benchmarks, I was easily able to see message queues of size ~3k-4k in NetworkProcess even without the receiver process being suspended. So we may need to consider upping this limit so that it doesn't become too noisy. But I think we can start with this limit.

Will up to 1024 for now. We can up if we becomes too noisy later. But it won't be more than once every 30 seconds so I am not too worried about noise at the moment.

bnham · 2023-05-01T22:31:24Z

Source/WebKit/Platform/IPC/Connection.cpp

+                , 0
+#endif
+            );
+            m_outgoingMessageQueueIsGrowingLargeCallback();


Maybe this should be called outside of the block that holds the lock?

Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp

webkit-early-warning-system · 2023-05-01T22:55:01Z

EWS run on previous version of this PR (hash 7ad9257)

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

bnham

LGTM

bnham · 2023-05-01T23:37:31Z

Source/WebKit/Platform/IPC/Connection.cpp

+    }
+
+    if (shouldNotifyOfQueueGrowingLarge) {
+        RELEASE_LOG_ERROR(IPC, "Connection::sendMessage(): Too many messages (%lu) in the queue to remote PID: %d, notifying client", outgoingMessagesCount


nit: %zu for size_t

webkit-early-warning-system · 2023-05-01T23:48:22Z

EWS run on current version of this PR (hash 966aea0)

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

…eue becomes too large https://bugs.webkit.org/show_bug.cgi?id=256181 Reviewed by Ben Nham. Temporarily wake up remote process if outgoing IPC message queue becomes too large, to avoid potentially bad memory growth when the remote process is suspended. This currently only works for AuxiliaryProcessProxy, so IPC from the UIProcess to child processes. However, we may want to consider extending this to IPC from the network process to WebProcesses in a follow-up. In the current proposal, we wake up the destination process for 3 seconds when its outgoing message queue has reached 1024 messages. We do so at most once every 30 seconds. * Source/WebKit/Platform/IPC/Connection.cpp: (IPC::Connection::setOutgoingMessageQueueIsGrowingLargeCallback): (IPC::Connection::invalidate): (IPC::Connection::sendMessage): * Source/WebKit/Platform/IPC/Connection.h: * Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp: (WebKit::AuxiliaryProcessProxy::AuxiliaryProcessProxy): (WebKit::AuxiliaryProcessProxy::didFinishLaunching): (WebKit::AuxiliaryProcessProxy::outgoingMessageQueueIsGrowingLarge): * Source/WebKit/UIProcess/AuxiliaryProcessProxy.h: Canonical link: https://commits.webkit.org/263570@main

webkit-commit-queue · 2023-05-02T02:14:20Z

Committed 263570@main (5183343): https://commits.webkit.org/263570@main

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

cdumez self-assigned this May 1, 2023

cdumez added the WebKit2 Bugs relating to the WebKit2 API layer label May 1, 2023

cdumez force-pushed the 256181_AuxiliaryProcessProxy_wake_up_remote_process_for_ipc branch from 17a41bd to 9e5d96d Compare May 1, 2023 21:59

bnham reviewed May 1, 2023

View reviewed changes

cdumez force-pushed the 256181_AuxiliaryProcessProxy_wake_up_remote_process_for_ipc branch from 9e5d96d to 7ad9257 Compare May 1, 2023 22:54

cdumez requested a review from geoffreygaren May 1, 2023 23:24

bnham approved these changes May 1, 2023

View reviewed changes

cdumez force-pushed the 256181_AuxiliaryProcessProxy_wake_up_remote_process_for_ipc branch from 7ad9257 to 966aea0 Compare May 1, 2023 23:41

cdumez added the merge-queue Applied to send a pull request to merge-queue label May 2, 2023

webkit-commit-queue force-pushed the 256181_AuxiliaryProcessProxy_wake_up_remote_process_for_ipc branch from 966aea0 to 5183343 Compare May 2, 2023 02:14

webkit-commit-queue merged commit 5183343 into WebKit:main May 2, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cocoa] Temporarily wake up remote process if outgoing IPC message queue becomes too large #13340

[Cocoa] Temporarily wake up remote process if outgoing IPC message queue becomes too large #13340

cdumez commented May 1, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 1, 2023 •

edited

webkit-early-warning-system commented May 1, 2023 •

edited

bnham May 1, 2023

cdumez May 1, 2023

bnham May 1, 2023

cdumez May 1, 2023

webkit-early-warning-system commented May 1, 2023 •

edited

bnham left a comment

bnham May 1, 2023

webkit-early-warning-system commented May 1, 2023 •

edited

webkit-commit-queue commented May 2, 2023

		#endif

		constexpr size_t largeOutgoingMessageQueueCountThreshold { 500 };

[Cocoa] Temporarily wake up remote process if outgoing IPC message queue becomes too large #13340

[Cocoa] Temporarily wake up remote process if outgoing IPC message queue becomes too large #13340

Conversation

cdumez commented May 1, 2023 • edited by webkit-early-warning-system

5183343

webkit-early-warning-system commented May 1, 2023 • edited

webkit-early-warning-system commented May 1, 2023 • edited

bnham May 1, 2023

Choose a reason for hiding this comment

cdumez May 1, 2023

Choose a reason for hiding this comment

bnham May 1, 2023

Choose a reason for hiding this comment

cdumez May 1, 2023

Choose a reason for hiding this comment

webkit-early-warning-system commented May 1, 2023 • edited

bnham left a comment

Choose a reason for hiding this comment

bnham May 1, 2023

Choose a reason for hiding this comment

webkit-early-warning-system commented May 1, 2023 • edited

webkit-commit-queue commented May 2, 2023

cdumez commented May 1, 2023 •

edited by webkit-early-warning-system

`5183343`

webkit-early-warning-system commented May 1, 2023 •

edited

webkit-early-warning-system commented May 1, 2023 •

edited

webkit-early-warning-system commented May 1, 2023 •

edited

webkit-early-warning-system commented May 1, 2023 •

edited