Summary: | Too much CPU time is spent under MemoryPressureHandler::currentMemoryUsagePolicy() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | Layout and Rendering | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bfulgham, changseok, esprehn+autocc, ews-watchlist, fred.wang, ggaren, glenn, kondapallykalyan, nham, pdr, simon.fraser, webkit-bug-importer, zalan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Chris Dumez
2021-06-14 16:56:33 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. Created attachment 431904 [details]
Patch
Comment on attachment 431904 [details]
Patch
10s seems too long. Maybe 1s or 2s?
Or we could do it once per rendering update. Created attachment 431906 [details]
Patch
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). (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. (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). (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. (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. 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]. |