Bug 224798 - Make sure we don't exit the GPUProcess too frequently while under memory pressure
Summary: Make sure we don't exit the GPUProcess too frequently while under memory pres...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
: 224778 (view as bug list)
Depends on:
Blocks: 224829
  Show dependency treegraph
 
Reported: 2021-04-19 20:05 PDT by Chris Dumez
Modified: 2021-04-23 02:48 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.53 KB, patch)
2021-04-19 20:11 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-04-19 20:05:55 PDT
Make sure we don't exit the GPUProcess too frequently while under memory pressure.
Comment 1 Chris Dumez 2021-04-19 20:11:19 PDT
Created attachment 426514 [details]
Patch
Comment 2 Darin Adler 2021-04-20 08:09:57 PDT
Comment on attachment 426514 [details]
Patch

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

> Source/WebKit/GPUProcess/GPUProcess.cpp:136
> +    // To avoid exiting the GPUProcess too aggressively while under memory pressure, we don't exit if we've been running
> +    // for less than 5 seconds. In case of simulated memory pressure, we ignore this rule to avoid generating flakiness
> +    // in our benchmarks and tests.
> +    if ((MonotonicTime::now() - m_creationTime) < 5_s && !MemoryPressureHandler::singleton().isSimulatingMemoryPressure())
> +        return false;

In cases like this I really like a comment somewhere explaining the choice of 5 seconds. One way to do that is to use a named constant at the top of the file and have the comment there focus on how we selected 5 seconds over other possible values. It’s sort of a "human being taking a pause" constant, I think?
Comment 3 EWS 2021-04-20 08:45:16 PDT
Committed r276305 (236787@main): <https://commits.webkit.org/236787@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426514 [details].
Comment 4 Geoffrey Garen 2021-04-20 11:09:13 PDT
If we know that we're under memory pressure, and we also know that it's too soon to exit the GPU Process right now, shouldn't we schedule an exit for a time that would be more appropriate?
Comment 5 Chris Dumez 2021-04-20 11:20:39 PDT
(In reply to Geoffrey Garen from comment #4)
> If we know that we're under memory pressure, and we also know that it's too
> soon to exit the GPU Process right now, shouldn't we schedule an exit for a
> time that would be more appropriate?

I think it would be appropriate although probably not critical.

I made this change because when the system is under memory pressure, the GPUProcess would exit very shortly after launch, even though it was about the be useful. If we launch the GPUProcess, then it indicates the WebProcess is about to use it very shortly. When the WebProcess, stops using it, we will already exit the GPUProcess if we are still under memory pressure and this time.

So the proposal to schedule an exit when receiving a memory pressure signal if we've just launched seems to be for this scenario:

The GPUProcess launched less than 5 seconds ago, it was used for a very short amount of time by the WebProcess (less than 5 seconds) and we did not exit when the WebProcess stopped using it because we were not under memory pressure and it had been less than 5 seconds. We then receive a memory pressure signal before the 5 seconds deadline and we don't exit. After the 5 seconds mark, we don't exit the GPUProcess if we are still under memory pressure, which is a bit unfortunate indeed. To cause the GPUProcess to exit at that point, we'd need either a new memory pressure signal OR a WebProcess would have to use the GPUProcess then stop using it.

I think based on this, I will follow-up and and such a timer.
Comment 6 Chris Dumez 2021-04-20 11:24:00 PDT
*** Bug 224778 has been marked as a duplicate of this bug. ***
Comment 7 Chris Dumez 2021-04-20 14:34:20 PDT
(In reply to Geoffrey Garen from comment #4)
> If we know that we're under memory pressure, and we also know that it's too
> soon to exit the GPU Process right now, shouldn't we schedule an exit for a
> time that would be more appropriate?

Following up to do this in Bug 224829.
Comment 8 Ling Ho 2021-04-23 02:48:57 PDT
rdar://76918236