RESOLVED FIXED 201329
Allow process cache to cache processes when using a non-default persistent WebsiteDataStore
https://bugs.webkit.org/show_bug.cgi?id=201329
Summary Allow process cache to cache processes when using a non-default persistent We...
Alex Christensen
Reported 2019-08-29 21:21:14 PDT
Allow process cache to cache processes when using a non-default persistent WebsiteDataStore
Attachments
Patch (1.82 KB, patch)
2019-08-29 21:32 PDT, Alex Christensen
no flags
Patch (3.60 KB, patch)
2019-08-30 10:39 PDT, Alex Christensen
cdumez: review+
Alex Christensen
Comment 1 2019-08-29 21:32:53 PDT
Chris Dumez
Comment 2 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.
Alex Christensen
Comment 3 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()
Chris Dumez
Comment 4 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 ?
Chris Dumez
Comment 5 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.
Alex Christensen
Comment 6 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.
Alex Christensen
Comment 7 2019-08-30 10:39:01 PDT
Alex Christensen
Comment 8 2019-08-30 13:24:13 PDT
Radar WebKit Bug Importer
Comment 9 2019-08-30 13:25:22 PDT
Note You need to log in before you can comment on or make changes to this bug.