Bug 227001 - Too much CPU time is spent under MemoryPressureHandler::currentMemoryUsagePolicy()
Summary: Too much CPU time is spent under MemoryPressureHandler::currentMemoryUsagePol...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-14 16:56 PDT by Chris Dumez
Modified: 2021-06-21 14:38 PDT (History)
13 users (show)

See Also:


Attachments
Patch (2.28 KB, patch)
2021-06-21 13:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (2.28 KB, patch)
2021-06-21 14:06 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-06-14 16:56:33 PDT
Too much CPU time is spent under MemoryPressureHandler::currentMemoryUsagePolicy().

Looking at profiles of our WebContent processes, we frequently see traces like these:
Sample Count, Samples %, Normalized CPU %, Symbol
31, 0.0%, 0.0%, WebCore::RenderLayerCompositor::cacheAcceleratedCompositingFlags() (in WebCore)
30, 0.0%, 0.0%,     WTF::MemoryPressureHandler::currentMemoryUsagePolicy() (in JavaScriptCore)
30, 0.0%, 0.0%,         task_info (in libsystem_kernel.dylib)

It is a decent amount of samples and shows several times per process.

We should try and minimize calls to WTF::MemoryPressureHandler::currentMemoryUsagePolicy() to reduce CPU usage.
Comment 1 Ben Nham 2021-06-14 21:48:48 PDT
Maybe we should just ditch computing the memory footprint on every call and map MemoryUsagePolicy to OS memory pressure levels (e.g. unrestricted => normal pressure, conservative => warning pressure, strict => critical pressure). Then we could just rely on the OS memory pressure notification handler to update MemoryUsagePolicy.

I guess the other obvious thing to do here is to cache the memoryFootprint for X seconds.
Comment 2 Chris Dumez 2021-06-21 13:55:08 PDT
Created attachment 431904 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-06-21 14:03:22 PDT
Comment on attachment 431904 [details]
Patch

10s seems too long. Maybe 1s or 2s?
Comment 4 Simon Fraser (smfr) 2021-06-21 14:03:46 PDT
Or we could do it once per rendering update.
Comment 5 Chris Dumez 2021-06-21 14:06:02 PDT
Created attachment 431906 [details]
Patch
Comment 6 Geoffrey Garen 2021-06-21 14:08:23 PDT
Can we pick the delay based on what it would take to make the % time in a representative profile negligible?

In the snippet posted here, the % time is already 0.0% (rounded, I assume), so I'm not sure any delay would be justified.

To say out loud what is perhaps implicit so far: memory pressure can change rapidly, so there is some benefit to responding as quickly as possible (but not so quickly that we waste CPU).
Comment 7 Simon Fraser (smfr) 2021-06-21 14:09:24 PDT
(In reply to Geoffrey Garen from comment #6)
> Can we pick the delay based on what it would take to make the % time in a
> representative profile negligible?
> 
> In the snippet posted here, the % time is already 0.0% (rounded, I assume),
> so I'm not sure any delay would be justified.
> 
> To say out loud what is perhaps implicit so far: memory pressure can change
> rapidly, so there is some benefit to responding as quickly as possible (but
> not so quickly that we waste CPU).

I suspect that the only cases where it's non-negligible are those where there are many forced layouts per rendering update. So once per rendering update feels right to me.
Comment 8 Chris Dumez 2021-06-21 14:09:32 PDT
(In reply to Geoffrey Garen from comment #6)
> Can we pick the delay based on what it would take to make the % time in a
> representative profile negligible?
> 
> In the snippet posted here, the % time is already 0.0% (rounded, I assume),
> so I'm not sure any delay would be justified.

This shows many times per profile. Not sure how to aggregate all the calls in the profiler.

> 
> To say out loud what is perhaps implicit so far: memory pressure can change
> rapidly, so there is some benefit to responding as quickly as possible (but
> not so quickly that we waste CPU).
Comment 9 Chris Dumez 2021-06-21 14:12:24 PDT
(In reply to Chris Dumez from comment #8)
> (In reply to Geoffrey Garen from comment #6)
> > Can we pick the delay based on what it would take to make the % time in a
> > representative profile negligible?
> > 
> > In the snippet posted here, the % time is already 0.0% (rounded, I assume),
> > so I'm not sure any delay would be justified.
> 
> This shows many times per profile. Not sure how to aggregate all the calls
> in the profiler.
> 
> > 
> > To say out loud what is perhaps implicit so far: memory pressure can change
> > rapidly, so there is some benefit to responding as quickly as possible (but
> > not so quickly that we waste CPU).

If the point is to respond to memory pressure, then we have the memory pressure warning/signal for that. This code relies on a certain % of the memory being used, which is not the same as memory pressure.
Comment 10 Chris Dumez 2021-06-21 14:15:45 PDT
(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #8)
> > (In reply to Geoffrey Garen from comment #6)
> > > Can we pick the delay based on what it would take to make the % time in a
> > > representative profile negligible?
> > > 
> > > In the snippet posted here, the % time is already 0.0% (rounded, I assume),
> > > so I'm not sure any delay would be justified.
> > 
> > This shows many times per profile. Not sure how to aggregate all the calls
> > in the profiler.
> > 
> > > 
> > > To say out loud what is perhaps implicit so far: memory pressure can change
> > > rapidly, so there is some benefit to responding as quickly as possible (but
> > > not so quickly that we waste CPU).
> 
> If the point is to respond to memory pressure, then we have the memory
> pressure warning/signal for that. This code relies on a certain % of the
> memory being used, which is not the same as memory pressure.

And this threshold appears to be 33% of RAM size of macOS and 50% of RAM size on iOS.
Comment 11 EWS 2021-06-21 14:36:34 PDT
Committed r279084 (239003@main): <https://commits.webkit.org/239003@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431906 [details].
Comment 12 Radar WebKit Bug Importer 2021-06-21 14:38:07 PDT
<rdar://problem/79581141>