Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2667343004:
Move purging memory from OnMemoryStateChange() to OnPurgeMemory() in discardable_memory (Closed)
DescriptionMove purging memory from OnMemoryStateChange() to OnPurgeMemory() in discardable_memory
We added OnPurgeMemory() callback to MemoryCoordinatorClient so that we
can separate memory purging from memory state transition (See [1] for
the motivation of this separation). This CL moves purging logic in
discardable_memory.
[1] https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer#!msg/project-trim/s96xSirL2Hs/18uq1zfHEgAJ
BUG=684287
Review-Url: https://codereview.chromium.org/2667343004
Cr-Commit-Position: refs/heads/master@{#448555}
Committed: https://chromium.googlesource.com/chromium/src/+/7032e2079c84274cc432a3ad2a09e96da8fcd162
Patch Set 1 : Don't reduce memory usage on state change #Patch Set 2 : Fix test #
Total comments: 3
Patch Set 3 : comment #
Messages
Total messages: 26 (21 generated)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bashi@chromium.org changed reviewers: + reveman@chromium.org
PTAL https://codereview.chromium.org/2667343004/diff/60001/components/discardable_... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2667343004/diff/60001/components/discardable_... components/discardable_memory/service/discardable_shared_memory_manager.cc:408: // Don't use SetMemoryLimit() as it free up existing allocations. As noted in https://cs.chromium.org/chromium/src/base/memory/memory_coordinator_client.h?..., we don't have to free up existing allocation on state change. OnPurgeMemory() is called as needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2667343004/diff/60001/components/discardable_... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2667343004/diff/60001/components/discardable_... components/discardable_memory/service/discardable_shared_memory_manager.cc:408: // Don't use SetMemoryLimit() as it free up existing allocations. nit: s/free/frees/ maybe also mention here that OnPurgeMemory() is called to actually free up memory.
Thanks for review! https://codereview.chromium.org/2667343004/diff/60001/components/discardable_... File components/discardable_memory/service/discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2667343004/diff/60001/components/discardable_... components/discardable_memory/service/discardable_shared_memory_manager.cc:408: // Don't use SetMemoryLimit() as it free up existing allocations. On 2017/02/07 03:34:07, reveman wrote: > nit: s/free/frees/ > > maybe also mention here that OnPurgeMemory() is called to actually free up > memory. Done.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2667343004/#ps80001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486441902980980,
"parent_rev": "efed310cc305f0d89f9d1aa49fb5e0d4ec9ff2cd", "commit_rev":
"7032e2079c84274cc432a3ad2a09e96da8fcd162"}
Message was sent while issue was closed.
Description was changed from ========== Move purging memory from OnMemoryStateChange() to OnPurgeMemory() in discardable_memory We added OnPurgeMemory() callback to MemoryCoordinatorClient so that we can separate memory purging from memory state transition (See [1] for the motivation of this separation). This CL moves purging logic in discardable_memory. [1] https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f... BUG=684287 ========== to ========== Move purging memory from OnMemoryStateChange() to OnPurgeMemory() in discardable_memory We added OnPurgeMemory() callback to MemoryCoordinatorClient so that we can separate memory purging from memory state transition (See [1] for the motivation of this separation). This CL moves purging logic in discardable_memory. [1] https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f... BUG=684287 Review-Url: https://codereview.chromium.org/2667343004 Cr-Commit-Position: refs/heads/master@{#448555} Committed: https://chromium.googlesource.com/chromium/src/+/7032e2079c84274cc432a3ad2a09... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7032e2079c84274cc432a3ad2a09... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
