Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2151693005:
[Chromecast] Use MemAvailable to compute memory pressure (Closed)
|
Created:
4 years, 5 months ago by halliwell Modified:
4 years, 5 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Use MemAvailable to compute memory pressure
Now this has been backported to most of our kernels, it should be the
most accurate way to assess memory pressure level.
These threshold values should be tweaked in future based on metrics.
BUG=620532
TEST=monitored MemAvailable in multiple scenarios (media playback as
well as background app)
Committed: https://crrev.com/be200eca4309e60f9d09739c3ff8fc92ead57c2e
Cr-Commit-Position: refs/heads/master@{#405652}
Patch Set 1 #
Total comments: 4
Patch Set 2 : nits + clang format #
Total comments: 2
Patch Set 3 : float->int #
Total comments: 2
Patch Set 4 : static_cast #Messages
Total messages: 18 (6 generated)
Description was changed from
==========
[Chromecast] Use MemAvailable to compute memory pressure
Now this has been backported to most of our kernels, it should be the
most accurate way to assess memory pressure level.
These threshold values should be tweaked in future based on metrics.
BUG=620532
TEST=monitored MemAvailable in multiple scenarios (media playback as
well as background app)
==========
to
==========
[Chromecast] Use MemAvailable to compute memory pressure
Now this has been backported to most of our kernels, it should be the
most accurate way to assess memory pressure level.
These threshold values should be tweaked in future based on metrics.
BUG=620532
TEST=monitored MemAvailable in multiple scenarios (media playback as
well as background app)
==========
halliwell@chromium.org changed reviewers: + derekjchow@chromium.org, wzhong@chromium.org
I added a bunch of info to the appendix of 'cast memory pressure handling' design doc to show how these initial thresholds were chosen. But, the thresholds should be tweaked based on metrics in future.
lgtm % nits https://codereview.chromium.org/2151693005/diff/1/chromecast/browser/cast_mem... File chromecast/browser/cast_memory_pressure_monitor.cc (right): https://codereview.chromium.org/2151693005/diff/1/chromecast/browser/cast_mem... chromecast/browser/cast_memory_pressure_monitor.cc:74: const float total_available = (info.available == 0) ? If the fallback is (free + buffers + cached), I would change the order of this tenary operation. https://codereview.chromium.org/2151693005/diff/1/chromecast/browser/cast_mem... chromecast/browser/cast_memory_pressure_monitor.cc:80: DCHECK(total > 0); DCHECK_GT?
https://codereview.chromium.org/2151693005/diff/1/chromecast/browser/cast_mem... File chromecast/browser/cast_memory_pressure_monitor.cc (right): https://codereview.chromium.org/2151693005/diff/1/chromecast/browser/cast_mem... chromecast/browser/cast_memory_pressure_monitor.cc:74: const float total_available = (info.available == 0) ? On 2016/07/14 19:09:27, derekjchow1 wrote: > If the fallback is (free + buffers + cached), I would change the order of this > tenary operation. Done. https://codereview.chromium.org/2151693005/diff/1/chromecast/browser/cast_mem... chromecast/browser/cast_memory_pressure_monitor.cc:80: DCHECK(total > 0); On 2016/07/14 19:09:27, derekjchow1 wrote: > DCHECK_GT? Done.
https://codereview.chromium.org/2151693005/diff/20001/chromecast/browser/cast... File chromecast/browser/cast_memory_pressure_monitor.cc (right): https://codereview.chromium.org/2151693005/diff/20001/chromecast/browser/cast... chromecast/browser/cast_memory_pressure_monitor.cc:74: const float total_available = Why is it a float? Shouldn't cast only when calculating ratio?
https://codereview.chromium.org/2151693005/diff/20001/chromecast/browser/cast... File chromecast/browser/cast_memory_pressure_monitor.cc (right): https://codereview.chromium.org/2151693005/diff/20001/chromecast/browser/cast... chromecast/browser/cast_memory_pressure_monitor.cc:74: const float total_available = On 2016/07/14 21:30:13, wzhong wrote: > Why is it a float? Shouldn't cast only when calculating ratio? Sure, I updated to just cast when calculating ratio.
https://codereview.chromium.org/2151693005/diff/40001/chromecast/browser/cast... File chromecast/browser/cast_memory_pressure_monitor.cc (right): https://codereview.chromium.org/2151693005/diff/40001/chromecast/browser/cast... chromecast/browser/cast_memory_pressure_monitor.cc:82: const float ratio = available / float(total); static_cast
https://codereview.chromium.org/2151693005/diff/40001/chromecast/browser/cast... File chromecast/browser/cast_memory_pressure_monitor.cc (right): https://codereview.chromium.org/2151693005/diff/40001/chromecast/browser/cast... chromecast/browser/cast_memory_pressure_monitor.cc:82: const float ratio = available / float(total); On 2016/07/14 23:55:25, wzhong wrote: > static_cast Sure, done.
lgtm
The CQ bit was checked by halliwell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derekjchow@chromium.org Link to the patchset: https://codereview.chromium.org/2151693005/#ps60001 (title: "static_cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
[Chromecast] Use MemAvailable to compute memory pressure
Now this has been backported to most of our kernels, it should be the
most accurate way to assess memory pressure level.
These threshold values should be tweaked in future based on metrics.
BUG=620532
TEST=monitored MemAvailable in multiple scenarios (media playback as
well as background app)
==========
to
==========
[Chromecast] Use MemAvailable to compute memory pressure
Now this has been backported to most of our kernels, it should be the
most accurate way to assess memory pressure level.
These threshold values should be tweaked in future based on metrics.
BUG=620532
TEST=monitored MemAvailable in multiple scenarios (media playback as
well as background app)
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
[Chromecast] Use MemAvailable to compute memory pressure
Now this has been backported to most of our kernels, it should be the
most accurate way to assess memory pressure level.
These threshold values should be tweaked in future based on metrics.
BUG=620532
TEST=monitored MemAvailable in multiple scenarios (media playback as
well as background app)
==========
to
==========
[Chromecast] Use MemAvailable to compute memory pressure
Now this has been backported to most of our kernels, it should be the
most accurate way to assess memory pressure level.
These threshold values should be tweaked in future based on metrics.
BUG=620532
TEST=monitored MemAvailable in multiple scenarios (media playback as
well as background app)
Committed: https://crrev.com/be200eca4309e60f9d09739c3ff8fc92ead57c2e
Cr-Commit-Position: refs/heads/master@{#405652}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/be200eca4309e60f9d09739c3ff8fc92ead57c2e Cr-Commit-Position: refs/heads/master@{#405652} |
