| Summary: | Allow process cache to cache processes when using a non-default persistent WebsiteDataStore | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cdumez, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Alex Christensen
2019-08-29 21:21:14 PDT
Created attachment 377675 [details]
Patch
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. 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() > 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
?
(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. 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. Created attachment 377724 [details]
Patch
|