Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
511383003:
Adding proper GetUsedMemoryInPercent calculation (Closed)
|
Created:
6 years, 3 months ago by Mr4D (OOO till 08-26) Modified:
6 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdding proper GetUsedMemoryInPercent calculation
This should be the memory pressure calculation as it is performed by the kernel today.
As discussed this is done to experiment with a more detailed resource management algorithm. Once this we have a satisfying solution this can be moved back.
BUG=381212
TEST=none (for this equation)
Committed: https://crrev.com/4ce9865875145e9ab3947b9bfc2b543347883a9b
Cr-Commit-Position: refs/heads/master@{#292779}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressed #
Total comments: 8
Patch Set 3 : addressed #Messages
Total messages: 18 (4 generated)
skuhne@chromium.org changed reviewers: + semenzato@chromium.org
Please have a look!
Looks good, but may need a few fixes. Thanks! https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... File athena/resource_manager/delegate/resource_manager_delegate.cc (right): https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:30: if (base::GetSystemMemoryInfo(&info) && info.total > 0 && info.free >= 0) { Not sure that you should check info.total and info.free. If GetSystemMemoryInfo returns true, info.total should have a sane value, and if not, it's not your job to report problems here i.e. the sanity check should already happen inside GetSystemMemoryInfo. Same for info.free. https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:31: // TODO(skuhne): Instead of adding the kernel memory pressure calculation Do you want to open a bug to keep track of this? https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:36: // Since swapp-able memory uses a non pre-deterministic compression and "swappable" may be better https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:37: // the compression creates it's own "dynamic" in the system, it gets its https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:44: // We don't know this value - it's kernel internal. Just use 50Mb. You're already copying kernel internals by making the same calculation as the kernel. https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:57: return std::min(0, std::max(percentage, 100)); available memory can be assumed to be positive, so you only have to check that available_memory < total_memory, and log an error if it is not.
Please have another look! https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... File athena/resource_manager/delegate/resource_manager_delegate.cc (right): https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:30: if (base::GetSystemMemoryInfo(&info) && info.total > 0 && info.free >= 0) { On 2014/08/29 15:37:36, Luigi Semenzato wrote: > Not sure that you should check info.total and info.free. If GetSystemMemoryInfo > returns true, info.total should have a sane value, and if not, it's not your job > to report problems here i.e. the sanity check should already happen inside > GetSystemMemoryInfo. Same for info.free. Done. https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:31: // TODO(skuhne): Instead of adding the kernel memory pressure calculation On 2014/08/29 15:37:36, Luigi Semenzato wrote: > Do you want to open a bug to keep track of this? Done. https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:36: // Since swapp-able memory uses a non pre-deterministic compression and On 2014/08/29 15:37:36, Luigi Semenzato wrote: > "swappable" may be better Done. https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:37: // the compression creates it's own "dynamic" in the system, it gets On 2014/08/29 15:37:36, Luigi Semenzato wrote: > its Done. https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:44: // We don't know this value - it's kernel internal. On 2014/08/29 15:37:36, Luigi Semenzato wrote: > Just use 50Mb. You're already copying kernel internals by making the same > calculation as the kernel. Done. https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:57: return std::min(0, std::max(percentage, 100)); Done. I might be wrong, but what if the compression ratio comes out to be much higher then expected. Wouldn't it be possible to have then more avail then total memory?
LGTM, however I don't know if I have LGTM privileges for this Chromium code. Thanks! https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... File athena/resource_manager/delegate/resource_manager_delegate.cc (right): https://codereview.chromium.org/511383003/diff/1/athena/resource_manager/dele... athena/resource_manager/delegate/resource_manager_delegate.cc:57: return std::min(0, std::max(percentage, 100)); Ah, good question. This may be a concern in general but not with our typical numbers: our swap is at most 1.5 * RAM size, and divided by 4 it's 0.375 * RAM so... but OK, you can leave as is. Either way. On 2014/08/29 16:45:47, Mr4D wrote: > Done. I might be wrong, but what if the compression ratio comes out to be much > higher then expected. Wouldn't it be possible to have then more avail then total > memory?
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/511383003/20001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
skuhne@chromium.org changed reviewers: + mukai@chromium.org
Jun, can you do a review (apparently it is not good enough to have Luigi reviewing, even if I am the owner) Thanks!
https://codereview.chromium.org/511383003/diff/20001/athena/resource_manager/... File athena/resource_manager/delegate/resource_manager_delegate.cc (right): https://codereview.chromium.org/511383003/diff/20001/athena/resource_manager/... athena/resource_manager/delegate/resource_manager_delegate.cc:29: if (base::GetSystemMemoryInfo(&info)) { Early exit like if(!base::GetSystemMemoryInfo(&info)) { LOG(WARNING) << ...; return 0; } would be better. https://codereview.chromium.org/511383003/diff/20001/athena/resource_manager/... athena/resource_manager/delegate/resource_manager_delegate.cc:45: int min_file_memory = 50 * 1024; const int kMinFileMemory = ... https://codereview.chromium.org/511383003/diff/20001/athena/resource_manager/... athena/resource_manager/delegate/resource_manager_delegate.cc:50: file_memory -= info.dirty - min_file_memory; If dirty is less than 50MB, it will increase file_memory, right? should be std::max(0, info.dirty - ...)? https://codereview.chromium.org/511383003/diff/20001/athena/resource_manager/... athena/resource_manager/delegate/resource_manager_delegate.cc:54: info.free + info.swap_free / kSwapWeight + file_memory; or this could be std::min(file_memory, min_file_memory) here rather than operating above?
Please have another look! https://codereview.chromium.org/511383003/diff/20001/athena/resource_manager/... File athena/resource_manager/delegate/resource_manager_delegate.cc (right): https://codereview.chromium.org/511383003/diff/20001/athena/resource_manager/... athena/resource_manager/delegate/resource_manager_delegate.cc:29: if (base::GetSystemMemoryInfo(&info)) { On 2014/08/29 22:59:32, Jun Mukai wrote: > Early exit like if(!base::GetSystemMemoryInfo(&info)) { LOG(WARNING) << ...; > return 0; } > would be better. Done. https://codereview.chromium.org/511383003/diff/20001/athena/resource_manager/... athena/resource_manager/delegate/resource_manager_delegate.cc:45: int min_file_memory = 50 * 1024; On 2014/08/29 22:59:32, Jun Mukai wrote: > const int kMinFileMemory = ... Done. https://codereview.chromium.org/511383003/diff/20001/athena/resource_manager/... athena/resource_manager/delegate/resource_manager_delegate.cc:50: file_memory -= info.dirty - min_file_memory; sign got now corrected. https://codereview.chromium.org/511383003/diff/20001/athena/resource_manager/... athena/resource_manager/delegate/resource_manager_delegate.cc:54: info.free + info.swap_free / kSwapWeight + file_memory; According to our semenzato that isn't a problem. (I had it that way before). But the worst which can happen is that the fill goes above 100% - which is treated as above 90% anyways.
lgtm
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/511383003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as fbd31111d255884492668eb6ee52efb3a3ba8abd
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4ce9865875145e9ab3947b9bfc2b543347883a9b Cr-Commit-Position: refs/heads/master@{#292779} |
