Handle warning-level memory notifications more aggressively
Created attachment 426963 [details] Patch
Comment on attachment 426963 [details] Patch initial pass for testing
<rdar://problem/77091307>
Created attachment 427086 [details] Patch
Comment on attachment 427086 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427086&action=review > Source/WTF/wtf/PlatformEnableCocoa.h:353 > +#define ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING 1 Maybe ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING_IN_BACKGROUND_PROCESSES ? But overall, I am not sure we really need/want a build flag at all? What's the worry on iOS? Usually background processes on iOS will be suspended. If they are not because of some kind of process assertion, then why not treat these memory warnings as critical? I think we should try and keep things cross-platform as much as possible. > Source/WebKit/WebProcess/WebProcess.cpp:389 > + if (m_pagesInWindows.isEmpty() && critical == Critical::No) I have some concerns about m_pagesInWindows being possibly out of date. In particular, shouldn't we remove a WebPage from m_pagesInWindows in WebProcess::removeWebPage()? I'd feel a lot more confident stale pages could not be in m_pagesInWindows if we did that. I guess I could convinced myself that pageDidLeaveWindow MUST be called before WebProcess::removeWebPage() but it seems fragile.
(In reply to Chris Dumez from comment #5) > Comment on attachment 427086 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427086&action=review > > > Source/WTF/wtf/PlatformEnableCocoa.h:353 > > +#define ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING 1 > > Maybe ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING_IN_BACKGROUND_PROCESSES ? > > But overall, I am not sure we really need/want a build flag at all? What's > the worry on iOS? Usually background processes on iOS will be suspended. If > they are not because of some kind of process assertion, then why not treat > these memory warnings as critical? > I think we should try and keep things cross-platform as much as possible. Good point, I think I'll remove the ifdef. > > Source/WebKit/WebProcess/WebProcess.cpp:389 > > + if (m_pagesInWindows.isEmpty() && critical == Critical::No) > > I have some concerns about m_pagesInWindows being possibly out of date. In > particular, shouldn't we remove a WebPage from m_pagesInWindows in > WebProcess::removeWebPage()? I'd feel a lot more confident stale pages could > not be in m_pagesInWindows if we did that. I guess I could convinced myself > that pageDidLeaveWindow MUST be called before WebProcess::removeWebPage() > but it seems fragile. Hmm, I'm not sure how m_pagesInWindows would be out of date. WebProcess::removeWebPage calls WebProcess::pageWillLeaveWindow so that leaves m_pagesInWindows in a consistent state at the end of that run loop iteration. And the memory pressure handler also runs in the main run loop so it will see m_pagesInWindows in a consistent state. Am I missing something here?
(In reply to Ben Nham from comment #6) > (In reply to Chris Dumez from comment #5) > > Comment on attachment 427086 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=427086&action=review > > > > > Source/WTF/wtf/PlatformEnableCocoa.h:353 > > > +#define ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING 1 > > > > Maybe ENABLE_AGGRESSIVE_MEMORYPRESSURE_HANDLING_IN_BACKGROUND_PROCESSES ? > > > > But overall, I am not sure we really need/want a build flag at all? What's > > the worry on iOS? Usually background processes on iOS will be suspended. If > > they are not because of some kind of process assertion, then why not treat > > these memory warnings as critical? > > I think we should try and keep things cross-platform as much as possible. > > Good point, I think I'll remove the ifdef. > > > > Source/WebKit/WebProcess/WebProcess.cpp:389 > > > + if (m_pagesInWindows.isEmpty() && critical == Critical::No) > > > > I have some concerns about m_pagesInWindows being possibly out of date. In > > particular, shouldn't we remove a WebPage from m_pagesInWindows in > > WebProcess::removeWebPage()? I'd feel a lot more confident stale pages could > > not be in m_pagesInWindows if we did that. I guess I could convinced myself > > that pageDidLeaveWindow MUST be called before WebProcess::removeWebPage() > > but it seems fragile. > > Hmm, I'm not sure how m_pagesInWindows would be out of date. > WebProcess::removeWebPage calls WebProcess::pageWillLeaveWindow so that > leaves m_pagesInWindows in a consistent state at the end of that run loop > iteration. And the memory pressure handler also runs in the main run loop so > it will see m_pagesInWindows in a consistent state. Am I missing something > here? No, you are right. I missed that WebProcess::removeWebPage() was calling pageWillLeaveWindow(). All good.
Comment on attachment 427086 [details] Patch r=me but ideally without ifdef.
Created attachment 427095 [details] patch for landing
Committed r276618 (237047@main): <https://commits.webkit.org/237047@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427095 [details].