Bug 201329 - Allow process cache to cache processes when using a non-default persistent WebsiteDataStore
Summary: Allow process cache to cache processes when using a non-default persistent We...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-29 21:21 PDT by Alex Christensen
Modified: 2019-08-30 13:25 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.82 KB, patch)
2019-08-29 21:32 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (3.60 KB, patch)
2019-08-30 10:39 PDT, Alex Christensen
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-08-29 21:21:14 PDT
Allow process cache to cache processes when using a non-default persistent WebsiteDataStore
Comment 1 Alex Christensen 2019-08-29 21:32:53 PDT
Created attachment 377675 [details]
Patch
Comment 2 Chris Dumez 2019-08-30 09:29:51 PDT
Comment on attachment 377675 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377675&action=review

> Source/WebKit/UIProcess/WebProcessCache.cpp:69
> +    if (sessionID.isEphemeral() && !process.processPool().hasPagesUsingWebsiteDataStore(process.websiteDataStore())) {

I am not sure we want to change this behavior. It currently matches the logic WebProcessPool::pageBeginUsingWebsiteDataStore() / WebProcessPool::pageEndUsingWebsiteDataStore(). Once there is no longer any page using a page using a non-default session, we remove the session from the network process. Once this is done, why do we want to keep caching WebContent processes for that session?

Your bug title claims to allow caching for WebContent processes for non-default persistent WebsiteDataStores. However, they are already cached as long as there are pages using the WebsiteDataStore in question. What your patch does is allow to cache such processes *after* that session has been removed in memory because there are no longer any pages using it. It is not clear to be we want to do this.
Comment 3 Alex Christensen 2019-08-30 09:39:47 PDT
I think I agree, but there's definitely something strange going on.  Could you try with the PR attached to rdar://problem/47030981 ?  We're also removing sessions from the NetworkProcess when I don't think we should be, exposing https://bugs.webkit.org/show_bug.cgi?id=201324 .  There's still some implicit assumption that the only persistent session is the default session.  I think it's related to a combination of WebProcessPool::pageBeginUsingWebsiteDataStore and the use of nonDefaultDataStores()
Comment 4 Chris Dumez 2019-08-30 09:44:08 PDT
> We're also removing sessions from the NetworkProcess when I don't think we should be.

Do you mean :
1. that you seem non-default persistent sessions getting destroyed even though there are still pages using it?
or
2. that we should keep non-default persistent sessions in memory even when they no longer have no pages, similarly to what we do for the default session

?
Comment 5 Chris Dumez 2019-08-30 09:46:42 PDT
(In reply to Chris Dumez from comment #4)
> > We're also removing sessions from the NetworkProcess when I don't think we should be.
> 
> Do you mean :
> 1. that you seem non-default persistent sessions getting destroyed even
> though there are still pages using it?
> or
> 2. that we should keep non-default persistent sessions in memory even when
> they no longer have no pages, similarly to what we do for the default session
> 
> ?

If we made it so that non-default persistent session stay in memory (i.e. update WebProcessPool::pageBeginUsingWebsiteDataStore() / WebProcessPool::pageEndUsingWebsiteDataStore(), so option 2) then I think the change you're suggesting would then make sense. And maybe it would perf in the scenario you're considering.
Comment 6 Alex Christensen 2019-08-30 09:50:29 PDT
Our use of nonDefaultDataStores() isn't related to the perf scenario I'm looking at.  I think option 2 might be what we need.  I'll look into it.
Comment 7 Alex Christensen 2019-08-30 10:39:01 PDT
Created attachment 377724 [details]
Patch
Comment 8 Alex Christensen 2019-08-30 13:24:13 PDT
http://trac.webkit.org/r249341
Comment 9 Radar WebKit Bug Importer 2019-08-30 13:25:22 PDT
<rdar://problem/54894208>