New issue
Advanced search Search tips

Issue 684287 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Introduce Purge() callback to MemoryCoordinatorClient

Project Member Reported by bashi@chromium.org, Jan 24 2017

Issue description

Currently MemoryCoordinatorClient has only one callback: OnMemoryStateChange(). Clients try to free up memory when the state is elevated (e.g. NORMAL -> SUSPENDED). After the discussion[1], we realized that purging under memory pressure might not be a good idea. We are going to add Purge() callback to separate "free up memory" from state changes. 

Specifically, we are going to do followings:
- Add Purge() to MemoryCoordinatorClient
- For each existing clients, move logic from OnStateChange(SUSPENDED) to Purge()

[1] https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer#!msg/project-trim/s96xSirL2Hs/18uq1zfHEgAJ

 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7f5cbbe7f704ea74d57b7413a7b10b7a1b68750

commit a7f5cbbe7f704ea74d57b7413a7b10b7a1b68750
Author: bashi <bashi@chromium.org>
Date: Mon Jan 30 01:05:29 2017

Add OnPurgeMemory() to MemoryCoordinatorClient

Before this CL, MemoryCoordinatorClient has only one callback called
OnMemoryStateChange(). Clients try to free up memory when state change
happens (e.g. NORMAL -> SUSPENDED). This may not be a good strategy on
some platforms because we may touch compressed pages (see [1] for details).

This CL add another callback called OnPurgeMemory() to separate logic
for purging existing memory from memory state changes. This way we can
build flexible strategies for handling memory pressure.

[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/2655083003
Cr-Commit-Position: refs/heads/master@{#446951}

[modify] https://crrev.com/a7f5cbbe7f704ea74d57b7413a7b10b7a1b68750/base/BUILD.gn
[modify] https://crrev.com/a7f5cbbe7f704ea74d57b7413a7b10b7a1b68750/base/memory/memory_coordinator_client.h
[modify] https://crrev.com/a7f5cbbe7f704ea74d57b7413a7b10b7a1b68750/base/memory/memory_coordinator_client_registry.cc
[modify] https://crrev.com/a7f5cbbe7f704ea74d57b7413a7b10b7a1b68750/base/memory/memory_coordinator_client_registry.h
[add] https://crrev.com/a7f5cbbe7f704ea74d57b7413a7b10b7a1b68750/base/memory/memory_coordinator_client_registry_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Feb 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/110841cafec3a76ab7446dc7c59107572a2fc68f

commit 110841cafec3a76ab7446dc7c59107572a2fc68f
Author: bashi <bashi@chromium.org>
Date: Tue Feb 07 00:50:50 2017

Add ChildMemoryCoordinator::PurgeMemory()

In crrev.com/2655083003 we added OnPurgeMemory() to
MemoryCoordinatorClient and we are moving purging logic from
OnMemoryStateChange() to OnPurgeMemory(). To make this transition easy,
this CL adds PurgeMemory() to ChildMemoryCoordinator. This method calls
both OnPurgeMemory() and OnMemoryStateChange(SUSPENDED). After purging
logic in all clients move to OnPurgeMemory(), we can remove
OnMemoryStateChange() call.

BUG= 684287 

Review-Url: https://codereview.chromium.org/2669323002
Cr-Commit-Position: refs/heads/master@{#448475}

[modify] https://crrev.com/110841cafec3a76ab7446dc7c59107572a2fc68f/content/child/memory/child_memory_coordinator_impl.cc
[modify] https://crrev.com/110841cafec3a76ab7446dc7c59107572a2fc68f/content/child/memory/child_memory_coordinator_impl.h
[modify] https://crrev.com/110841cafec3a76ab7446dc7c59107572a2fc68f/content/renderer/render_thread_impl.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Feb 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f9da465ceffbabd47592b4142b4a4d293db7d685

commit f9da465ceffbabd47592b4142b4a4d293db7d685
Author: bashi <bashi@chromium.org>
Date: Tue Feb 07 04:36:52 2017

Replace OnMemoryStateChange() with OnPurgeMemory() in RendererFrameManager

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 replaces OnMemoryStateChange()
with OnPurgeMemory() in RendererFrameManager because it just purges
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/2675233003
Cr-Commit-Position: refs/heads/master@{#448544}

[modify] https://crrev.com/f9da465ceffbabd47592b4142b4a4d293db7d685/content/browser/renderer_host/renderer_frame_manager.cc
[modify] https://crrev.com/f9da465ceffbabd47592b4142b4a4d293db7d685/content/browser/renderer_host/renderer_frame_manager.h

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7032e2079c84274cc432a3ad2a09e96da8fcd162

commit 7032e2079c84274cc432a3ad2a09e96da8fcd162
Author: bashi <bashi@chromium.org>
Date: Tue Feb 07 05:31:38 2017

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=footer#!msg/project-trim/s96xSirL2Hs/18uq1zfHEgAJ

BUG= 684287 

Review-Url: https://codereview.chromium.org/2667343004
Cr-Commit-Position: refs/heads/master@{#448555}

[modify] https://crrev.com/7032e2079c84274cc432a3ad2a09e96da8fcd162/components/discardable_memory/service/discardable_shared_memory_manager.cc
[modify] https://crrev.com/7032e2079c84274cc432a3ad2a09e96da8fcd162/components/discardable_memory/service/discardable_shared_memory_manager.h

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56b23f3064d47916ee13dadbbb7e87e2d15b80fd

commit 56b23f3064d47916ee13dadbbb7e87e2d15b80fd
Author: bashi <bashi@chromium.org>
Date: Thu Feb 09 01:24:57 2017

Replace OnMemoryStateChange() with OnPurgeMemory() in net/

When we implemented MemoryCoordinatorClients in net/, there was only one
callback called OnMemoryStateChange(). Recently we added another
callback called OnPurgeMemory() to handle "existing allocations" and
"future allocations" separately (See [1] for the motivation). This CL
replaces OnMemoryStateChange() with OnPurgeMemory() because all clients
in net/ actually free up memory.

Note: MemoryCoordinatorClient and MemoryPressureListener won't be
enabled at the same time. This means that either OnMemoryPressure() or
OnPurgeMemory() can be called.

[1] https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer#!msg/project-trim/s96xSirL2Hs/18uq1zfHEgAJ

BUG= 684287 , 617492 

Review-Url: https://codereview.chromium.org/2679903002
Cr-Commit-Position: refs/heads/master@{#449178}

[modify] https://crrev.com/56b23f3064d47916ee13dadbbb7e87e2d15b80fd/net/http/http_network_session.cc
[modify] https://crrev.com/56b23f3064d47916ee13dadbbb7e87e2d15b80fd/net/http/http_network_session.h
[modify] https://crrev.com/56b23f3064d47916ee13dadbbb7e87e2d15b80fd/net/sdch/sdch_owner.cc
[modify] https://crrev.com/56b23f3064d47916ee13dadbbb7e87e2d15b80fd/net/sdch/sdch_owner.h
[modify] https://crrev.com/56b23f3064d47916ee13dadbbb7e87e2d15b80fd/net/ssl/ssl_client_session_cache.cc
[modify] https://crrev.com/56b23f3064d47916ee13dadbbb7e87e2d15b80fd/net/ssl/ssl_client_session_cache.h

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a8c0d87fb51b053a25e3cebfde5ccbd94a24f10c

commit a8c0d87fb51b053a25e3cebfde5ccbd94a24f10c
Author: bashi <bashi@chromium.org>
Date: Thu Feb 09 02:11:23 2017

memory coordinator: Call PurgeMemory() after Notify(SUSPENDED)

Currently we tentatively call PurgeMemory() before Notify(SUSPENDED).
The order should be reversed as there could be cache allocations before
suspending (throttling) task is actually done, as Eric mentioned[1].

[1] https://codereview.chromium.org/2664703002/#msg15

BUG= 684287 

Review-Url: https://codereview.chromium.org/2685933003
Cr-Commit-Position: refs/heads/master@{#449189}

[modify] https://crrev.com/a8c0d87fb51b053a25e3cebfde5ccbd94a24f10c/content/child/memory/child_memory_coordinator_impl.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c841b76ac7d8465283eb13e87d05388004a6f6bf

commit c841b76ac7d8465283eb13e87d05388004a6f6bf
Author: bashi <bashi@chromium.org>
Date: Fri Feb 10 00:15:49 2017

Move purging from OnMemoryState to OnPurgeMemory() in cc/

We are going to separate memory purging logic from memory state changes.
This CL moves purging from OnMemoryState() to OnPurgeMemory() in cc/.

BUG= 684287 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2664703002
Cr-Commit-Position: refs/heads/master@{#449484}

[modify] https://crrev.com/c841b76ac7d8465283eb13e87d05388004a6f6bf/cc/raster/staging_buffer_pool.cc
[modify] https://crrev.com/c841b76ac7d8465283eb13e87d05388004a6f6bf/cc/raster/staging_buffer_pool.h
[modify] https://crrev.com/c841b76ac7d8465283eb13e87d05388004a6f6bf/cc/resources/resource_pool.cc
[modify] https://crrev.com/c841b76ac7d8465283eb13e87d05388004a6f6bf/cc/resources/resource_pool.h
[modify] https://crrev.com/c841b76ac7d8465283eb13e87d05388004a6f6bf/cc/resources/resource_pool_unittest.cc
[modify] https://crrev.com/c841b76ac7d8465283eb13e87d05388004a6f6bf/cc/tiles/gpu_image_decode_cache.cc
[modify] https://crrev.com/c841b76ac7d8465283eb13e87d05388004a6f6bf/cc/tiles/gpu_image_decode_cache.h
[modify] https://crrev.com/c841b76ac7d8465283eb13e87d05388004a6f6bf/cc/tiles/gpu_image_decode_cache_unittest.cc
[modify] https://crrev.com/c841b76ac7d8465283eb13e87d05388004a6f6bf/cc/tiles/software_image_decode_cache.cc
[modify] https://crrev.com/c841b76ac7d8465283eb13e87d05388004a6f6bf/cc/tiles/software_image_decode_cache.h

Comment 10 by bashi@chromium.org, Feb 10 2017

Status: Fixed (was: Assigned)

Comment 11 by bashi@chromium.org, Feb 10 2017

Status: Started (was: Fixed)
Re-opening as I still need to do a few things for Purge()
- Move purging in dom_storage: https://codereview.chromium.org/2671303004/
- Add a mojo method for ChildMemoryCoordinator

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/492af7d3b599bf9e2a407d7e55e35617854c4e32

commit 492af7d3b599bf9e2a407d7e55e35617854c4e32
Author: bashi <bashi@chromium.org>
Date: Tue Feb 14 04:21:23 2017

Replace OnMemoryStateChange() with OnPurgeMemory() in dom_storage

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 replaces OnMemoryStateChange()
with OnPurgeMemory() in DOMStorageContextWrapper because it just purges
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/2671303004
Cr-Commit-Position: refs/heads/master@{#450242}

[modify] https://crrev.com/492af7d3b599bf9e2a407d7e55e35617854c4e32/content/browser/dom_storage/dom_storage_context_wrapper.cc
[modify] https://crrev.com/492af7d3b599bf9e2a407d7e55e35617854c4e32/content/browser/dom_storage/dom_storage_context_wrapper.h

Project Member

Comment 14 by bugdroid1@chromium.org, Feb 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a3481c035a42c8fef365f2f9d94c78305cee048

commit 7a3481c035a42c8fef365f2f9d94c78305cee048
Author: bashi <bashi@chromium.org>
Date: Tue Feb 14 09:20:13 2017

Don't notify SUSPENDED state in ChildMemoryCoordinator::PurgeMemory()

All purging logic in MemoryCoordinatorClients has moved to
OnPurgeMemory(). We don't need to call Notifiy(SUSPENDED) anymore.

BUG= 684287 

Review-Url: https://codereview.chromium.org/2695923003
Cr-Commit-Position: refs/heads/master@{#450300}

[modify] https://crrev.com/7a3481c035a42c8fef365f2f9d94c78305cee048/content/child/memory/child_memory_coordinator_impl.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Feb 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c80d2d2b0fcceac67a16bf6ffb516ba5c0023230

commit c80d2d2b0fcceac67a16bf6ffb516ba5c0023230
Author: wez <wez@chromium.org>
Date: Thu Feb 16 03:28:19 2017

Revert of Add ChildMemoryCoordinator::PurgeMemory() (patchset #2 id:20001 of https://codereview.chromium.org/2669323002/ )

Reason for revert:
Appears to be the root cause of crbug.com/692509 (renderer crashes in RenderThreadImpl::OnProcessPurgeAndSuspend) - although the timeline doesn't match up, the change to RenderThreadImpl appears incorrect by inspection (see my comment in the original CL).

Original issue's description:
> Add ChildMemoryCoordinator::PurgeMemory()
>
> In crrev.com/2655083003 we added OnPurgeMemory() to
> MemoryCoordinatorClient and we are moving purging logic from
> OnMemoryStateChange() to OnPurgeMemory(). To make this transition easy,
> this CL adds PurgeMemory() to ChildMemoryCoordinator. This method calls
> both OnPurgeMemory() and OnMemoryStateChange(SUSPENDED). After purging
> logic in all clients move to OnPurgeMemory(), we can remove
> OnMemoryStateChange() call.
>
> BUG= 684287 
>
> Review-Url: https://codereview.chromium.org/2669323002
> Cr-Commit-Position: refs/heads/master@{#448475}
> Committed: https://chromium.googlesource.com/chromium/src/+/110841cafec3a76ab7446dc7c59107572a2fc68f

NOTRY=true
TBR=tasak@google.com,jam@chromium.org,haraken@chromium.org,bashi@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 684287 

Review-Url: https://codereview.chromium.org/2693223003
Cr-Commit-Position: refs/heads/master@{#450859}

[modify] https://crrev.com/c80d2d2b0fcceac67a16bf6ffb516ba5c0023230/content/renderer/render_thread_impl.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Feb 16 2017

Labels: merge-merged-3013
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1145a7dc82f06717a15b35f36442f3b1e5b6b49

commit c1145a7dc82f06717a15b35f36442f3b1e5b6b49
Author: Kenichi Ishibashi <bashi@chromium.org>
Date: Thu Feb 16 04:30:24 2017

Revert of Add ChildMemoryCoordinator::PurgeMemory() (patchset #2 id:20001 of https://codereview.chromium.org/2669323002/ )

Reason for revert:
Appears to be the root cause of crbug.com/692509 (renderer crashes in RenderThreadImpl::OnProcessPurgeAndSuspend) - although the timeline doesn't match up, the change to RenderThreadImpl appears incorrect by inspection (see my comment in the original CL).

Original issue's description:
> Add ChildMemoryCoordinator::PurgeMemory()
>
> In crrev.com/2655083003 we added OnPurgeMemory() to
> MemoryCoordinatorClient and we are moving purging logic from
> OnMemoryStateChange() to OnPurgeMemory(). To make this transition easy,
> this CL adds PurgeMemory() to ChildMemoryCoordinator. This method calls
> both OnPurgeMemory() and OnMemoryStateChange(SUSPENDED). After purging
> logic in all clients move to OnPurgeMemory(), we can remove
> OnMemoryStateChange() call.
>
> BUG= 684287 
>
> Review-Url: https://codereview.chromium.org/2669323002
> Cr-Commit-Position: refs/heads/master@{#448475}
> Committed: https://chromium.googlesource.com/chromium/src/+/110841cafec3a76ab7446dc7c59107572a2fc68f

NOTRY=true
TBR=tasak@google.com,jam@chromium.org,haraken@chromium.org,bashi@chromium.org
BUG= 684287 

Review-Url: https://codereview.chromium.org/2693223003
Cr-Commit-Position: refs/heads/master@{#450859}
(cherry picked from commit c80d2d2b0fcceac67a16bf6ffb516ba5c0023230)

Review-Url: https://codereview.chromium.org/2697143003 .
Cr-Commit-Position: refs/branch-heads/3013@{#3}
Cr-Branched-From: 1645e63d771e2ee049d465ce70bca163fd9f5bcc-refs/heads/master@{#450530}

[modify] https://crrev.com/c1145a7dc82f06717a15b35f36442f3b1e5b6b49/content/renderer/render_thread_impl.cc

Comment 17 by bashi@chromium.org, Feb 16 2017

Status: Fixed (was: Started)

Sign in to add a comment