Changeset 237735 in webkit

Timestamp:

Nov 2, 2018 8:59:23 AM (5 years ago)

Author:

Chris Dumez

Message:

[PSON] Reuse SuspendedPages' process when possible, for performance
​https://bugs.webkit.org/show_bug.cgi?id=191166

Reviewed by Geoffrey Garen.

Source/WebKit:

When process-swapping check if there is an existing SuspendedPage for the domain we're going to.
If there is, use this SuspendedPage's process for the navigation instead of a fresh new process.
This change should be beneficial for performance as it:

• Avoids spinning up a new process (CPU & memory cost)
• Likely better leverages caches since this process already loaded this domain in the past

Due to current limitations, using a SuspendedPage's proxy may consume the SuspendedPage, which
means that it can no longer be used for PageCache on history navigations. We need to do this when
the SuspendedPageProxy in question was created for the current WebPageProxy because:

• This SuspendedPageProxy's process already has a suspended WebPage with this WebPageProxy's pageID

and

• We do not currently support having more than one WebPage with a given pageID within a single WebProcess.

• UIProcess/SuspendedPageProxy.cpp:

(WebKit::SuspendedPageProxy::SuspendedPageProxy):

• UIProcess/SuspendedPageProxy.h:
• UIProcess/WebProcessPool.cpp:

(WebKit::WebProcessPool::processForNavigationInternal):

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/SuspendedPageProxy.cpp (modified) (1 diff)
• Source/WebKit/UIProcess/SuspendedPageProxy.h (modified) (2 diffs)
• Source/WebKit/UIProcess/WebProcessPool.cpp (modified) (2 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (modified) (4 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2018-11-02  Chris Dumez  <cdumez@apple.com>
+
+        [PSON] Reuse SuspendedPages' process when possible, for performance
+        https://bugs.webkit.org/show_bug.cgi?id=191166
+
+        Reviewed by Geoffrey Garen.
+
+        When process-swapping check if there is an existing SuspendedPage for the domain we're going to.
+        If there is, use this SuspendedPage's process for the navigation instead of a fresh new process.
+        This change should be beneficial for performance as it:
+        - Avoids spinning up a new process (CPU & memory cost)
+        - Likely better leverages caches since this process already loaded this domain in the past
+
+        Due to current limitations, using a SuspendedPage's proxy may consume the SuspendedPage, which
+        means that it can no longer be used for PageCache on history navigations. We need to do this when
+        the SuspendedPageProxy in question was created for the current WebPageProxy because:
+        - This SuspendedPageProxy's process already has a suspended WebPage with this WebPageProxy's pageID
+        and
+        - We do not currently support having more than one WebPage with a given pageID within a single
+          WebProcess.
+
+        * UIProcess/SuspendedPageProxy.cpp:
+        (WebKit::SuspendedPageProxy::SuspendedPageProxy):
+        * UIProcess/SuspendedPageProxy.h:
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::processForNavigationInternal):
+
 2018-11-01  Fujii Hironori  <Hironori.Fujii@sony.com>
 

trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp

@@ -79 +79 @@
     , m_process(WTFMove(process))
     , m_mainFrameID(mainFrameID)
-    , m_origin(SecurityOriginData::fromURL({ { }, item.url() }))
+    , m_registrableDomain(toRegistrableDomain(URL(URL(), item.url())))
 {
     item.setSuspendedPage(*this);

trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h

@@ -45 +45 @@
     WebProcessProxy& process() { return m_process.get(); }
     uint64_t mainFrameID() const { return m_mainFrameID; }
-    const WebCore::SecurityOriginData& origin() const { return m_origin; }
+    const String& registrableDomain() const { return m_registrableDomain; }
 
     void unsuspend();
@@ -63 +63 @@
     Ref<WebProcessProxy> m_process;
     uint64_t m_mainFrameID;
-    WebCore::SecurityOriginData m_origin;
+    String m_registrableDomain;
 
     bool m_isSuspended { true };

trunk/Source/WebKit/UIProcess/WebProcessPool.cpp

@@ -2186 +2186 @@
         reason = "Process swap was requested by the client"_s;
     
+    auto registrableDomain = toRegistrableDomain(targetURL);
     if (m_configuration->alwaysKeepAndReuseSwappedProcesses()) {
-        auto registrableDomain = toRegistrableDomain(targetURL);
         LOG(ProcessSwapping, "(ProcessSwapping) Considering re-use of a previously cached process for domain %s", registrableDomain.utf8().data());
 
@@ -2197 +2197 @@
                 // In the case where this WebProcess has a SuspendedPageProxy for this WebPage, we can throw it away to support
                 // WebProcess re-use.
-                // In the future it would be great to refactor-out this limitation.
-                page.process().processPool().removeAllSuspendedPageProxiesForPage(page);
+                // In the future it would be great to refactor-out this limitation (https://bugs.webkit.org/show_bug.cgi?id=191166).
+                m_suspendedPages.removeAllMatching([&](auto& suspendedPage) {
+                    return &suspendedPage->page() == &page && &suspendedPage->process() == process;
+                });
 
                 return makeRef(*process);
             }
         }
+    }
+
+    // Check if we have a suspended page for the given registrable domain and use its process if we do, for performance reasons.
+    auto it = m_suspendedPages.findIf([&](auto& suspendedPage) {
+        return suspendedPage->registrableDomain() == registrableDomain;
+    });
+    if (it != m_suspendedPages.end()) {
+        Ref<WebProcessProxy> process = (*it)->process();
+
+        // FIXME: If the SuspendedPage is for this page, then we need to destroy the suspended page as we do not support having
+        // multiple WebPages with the same ID in a given WebProcess currently (https://bugs.webkit.org/show_bug.cgi?id=191166).
+        if (&(*it)->page() == &page)
+            m_suspendedPages.remove(it);
+
+        if (&process->websiteDataStore() != &page.websiteDataStore())
+            process->send(Messages::WebProcess::AddWebsiteDataStore(page.websiteDataStore().parameters()), 0);
+
+        return process;
     }
 

trunk/Tools/ChangeLog

@@ +1 @@
+2018-11-02  Chris Dumez  <cdumez@apple.com>
+
+        [PSON] Reuse SuspendedPages' process when possible, for performance
+        https://bugs.webkit.org/show_bug.cgi?id=191166
+
+        Reviewed by Geoffrey Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2018-11-02  Zalan Bujtas  <zalan@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

@@ -1524 +1524 @@
     done = false;
 
-    auto pid1 = [webView _webProcessIdentifier];
+    auto webkitPID = [webView _webProcessIdentifier];
 
     request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
@@ -1532 +1532 @@
     done = false;
 
-    auto pid2 = [webView _webProcessIdentifier];
+    auto applePID = [webView _webProcessIdentifier];
+
+    // Verify the web pages are in different processes
+    EXPECT_NE(webkitPID, applePID);
 
     request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
@@ -1542 +1545 @@
     done = false;
 
-    auto pid3 = [webView _webProcessIdentifier];
-
-    // Verify the web pages are in different processes
-    EXPECT_NE(pid1, pid2);
-    EXPECT_NE(pid1, pid3);
-    EXPECT_NE(pid2, pid3);
+    // We should have gone back to the webkit.org process for this load since we reuse SuspendedPages' process when possible.
+    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
 
     // Verify the sessionStorage values were as expected
@@ -1553 +1552 @@
     EXPECT_TRUE([receivedMessages.get()[0] isEqualToString:@""]);
     EXPECT_TRUE([receivedMessages.get()[1] isEqualToString:@"I exist!"]);
+}
+
+TEST(ProcessSwap, ReuseSuspendedProcess)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    [processPoolConfiguration setProcessSwapsOnNavigation:YES];
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto webkitPID = [webView _webProcessIdentifier];
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main1.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto applePID = [webView _webProcessIdentifier];
+
+    EXPECT_NE(webkitPID, applePID);
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    // We should have gone back to the webkit.org process for this load since we reuse SuspendedPages' process when possible.
+    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main2.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    // We should have gone back to the apple.com process for this load since we reuse SuspendedPages' process when possible.
+    EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
 }