Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2336043004:
Android: Free GLHelper context ashmem when it makes sense (Closed)
|
Created:
4 years, 3 months ago by no sievers Modified:
4 years, 2 months ago Reviewers:
danakj CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid: Free GLHelper context ashmem when it makes sense
Free the 'mapped_memory' that is used in readbacks through
CopyFromCompositingSurface() when there is memory pressure
in the system (or no activities are running and a readback
completes).
This memory gets lazily allocated during the first readback
and it'd be the size of the texture after scaling. Freeing it
is harmless at a slight reallocation cost when the next readback
is triggered.
BUG=641962
TBR=boliu@chromium.org
Committed: https://crrev.com/98f7c133cdef22c26922c56371f84989f7a2cd60
Cr-Commit-Position: refs/heads/master@{#421357}
Patch Set 1 #Patch Set 2 : Android: Free GLHelper context ashmem when it makes sense #
Total comments: 15
Patch Set 3 : rebase #Patch Set 4 : address comments #
Total comments: 1
Messages
Total messages: 19 (9 generated)
Description was changed from ========== Android: Free GLHelper context ashmem when it makes sense Free the 'mapped_memory' that is used in readbacks through CopyFromCompositingSurface() when no activities are running or there is memory pressure in the system. This memory gets lazily allocated during the first readback and it'd be the size of the texture after scaling. Freeing it is harmless at a slight reallocation cost when the next readback is triggered. BUG=641962 ========== to ========== Android: Free GLHelper context ashmem when it makes sense Free the 'mapped_memory' that is used in readbacks through CopyFromCompositingSurface() when no activities are running or there is memory pressure in the system. This memory gets lazily allocated during the first readback and it'd be the size of the texture after scaling. Freeing it is harmless at a slight reallocation cost when the next readback is triggered. BUG=641962 ==========
sievers@chromium.org changed reviewers: + danakj@chromium.org
Dana, ptal, thanks!
Description was changed from ========== Android: Free GLHelper context ashmem when it makes sense Free the 'mapped_memory' that is used in readbacks through CopyFromCompositingSurface() when no activities are running or there is memory pressure in the system. This memory gets lazily allocated during the first readback and it'd be the size of the texture after scaling. Freeing it is harmless at a slight reallocation cost when the next readback is triggered. BUG=641962 ========== to ========== Android: Free GLHelper context ashmem when it makes sense Free the 'mapped_memory' that is used in readbacks through CopyFromCompositingSurface() when there is memory pressure in the system (or no activities are running and a readback completes). This memory gets lazily allocated during the first readback and it'd be the size of the texture after scaling. Freeing it is harmless at a slight reallocation cost when the next readback is triggered. BUG=641962 ==========
https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:210: &GLHelperHolder::OnApplicationStatusChanged, base::Unretained(this)))); Please leave a comment explaining why unretained this is safe https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:212: &GLHelperHolder::OnMemoryPressure, base::Unretained(this)))); ditto https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:223: provider_->FreeUnusedSharedMemory(); What if we just delete the helper instead? https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:250: should_aggressively_free_memory_ = true; I feel weird about 2 functions setting this independently, will this lead to some weird behaviour where one sets it and the other changes it and we get not what we hoped for? https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:273: return GetPostReadbackGLHelperHolder(true)->gl_helper(); can you put the true into a temp var to give it a name https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:277: GLHelperHolder* holder = GetPostReadbackGLHelperHolder(false); ditto for false https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:301: FreeUnusedGLHelperMemory(); What if there was more than one copy going? https://codereview.chromium.org/2336043004/diff/20001/content/common/gpu/clie... File content/common/gpu/client/context_provider_command_buffer.cc (right): https://codereview.chromium.org/2336043004/diff/20001/content/common/gpu/clie... content/common/gpu/client/context_provider_command_buffer.cc:122: void ContextProviderCommandBuffer::FreeUnusedSharedMemory() { Hm, idk about adding another way to free memory. Why isn't using ContextSupport::SetAggressivelyFreeResources enough?
https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:210: &GLHelperHolder::OnApplicationStatusChanged, base::Unretained(this)))); On 2016/09/13 22:44:58, danakj wrote: > Please leave a comment explaining why unretained this is safe Done. https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:212: &GLHelperHolder::OnMemoryPressure, base::Unretained(this)))); On 2016/09/13 22:44:58, danakj wrote: > ditto Done. https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:223: provider_->FreeUnusedSharedMemory(); On 2016/09/13 22:44:58, danakj wrote: > What if we just delete the helper instead? Yes, good idea and it works because IsLost() above will handle |gl_helper_ == null| and we'd recreate it lazily from the getter below. https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:250: should_aggressively_free_memory_ = true; On 2016/09/13 22:44:58, danakj wrote: > I feel weird about 2 functions setting this independently, will this lead to > some weird behaviour where one sets it and the other changes it and we get not > what we hoped for? Done. https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:273: return GetPostReadbackGLHelperHolder(true)->gl_helper(); On 2016/09/13 22:44:58, danakj wrote: > can you put the true into a temp var to give it a name Done. https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:277: GLHelperHolder* holder = GetPostReadbackGLHelperHolder(false); On 2016/09/13 22:44:58, danakj wrote: > ditto for false Done. https://codereview.chromium.org/2336043004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:301: FreeUnusedGLHelperMemory(); On 2016/09/13 22:44:58, danakj wrote: > What if there was more than one copy going? I think that's fine because it would only free *unused* memory until the last readback completes and we'd free the remainder.
LGTM
https://codereview.chromium.org/2336043004/diff/60001/content/common/gpu/clie... File content/common/gpu/client/context_provider_command_buffer.h (right): https://codereview.chromium.org/2336043004/diff/60001/content/common/gpu/clie... content/common/gpu/client/context_provider_command_buffer.h:67: void FreeUnusedSharedMemory(); FWIW I think this should be part of ContextSupport or something, but I'd rather merge it/replace somehow the ShouldAggressivelyFreeResources so I'm happy with this here instead of on the ContextSupport API for now.
The CQ bit was checked by sievers@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Android: Free GLHelper context ashmem when it makes sense Free the 'mapped_memory' that is used in readbacks through CopyFromCompositingSurface() when there is memory pressure in the system (or no activities are running and a readback completes). This memory gets lazily allocated during the first readback and it'd be the size of the texture after scaling. Freeing it is harmless at a slight reallocation cost when the next readback is triggered. BUG=641962 ========== to ========== Android: Free GLHelper context ashmem when it makes sense Free the 'mapped_memory' that is used in readbacks through CopyFromCompositingSurface() when there is memory pressure in the system (or no activities are running and a readback completes). This memory gets lazily allocated during the first readback and it'd be the size of the texture after scaling. Freeing it is harmless at a slight reallocation cost when the next readback is triggered. BUG=641962 TBR=boliu@chromium.org ==========
The CQ bit was unchecked by sievers@chromium.org
The CQ bit was checked by sievers@chromium.org
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 ========== Android: Free GLHelper context ashmem when it makes sense Free the 'mapped_memory' that is used in readbacks through CopyFromCompositingSurface() when there is memory pressure in the system (or no activities are running and a readback completes). This memory gets lazily allocated during the first readback and it'd be the size of the texture after scaling. Freeing it is harmless at a slight reallocation cost when the next readback is triggered. BUG=641962 TBR=boliu@chromium.org ========== to ========== Android: Free GLHelper context ashmem when it makes sense Free the 'mapped_memory' that is used in readbacks through CopyFromCompositingSurface() when there is memory pressure in the system (or no activities are running and a readback completes). This memory gets lazily allocated during the first readback and it'd be the size of the texture after scaling. Freeing it is harmless at a slight reallocation cost when the next readback is triggered. BUG=641962 TBR=boliu@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Android: Free GLHelper context ashmem when it makes sense Free the 'mapped_memory' that is used in readbacks through CopyFromCompositingSurface() when there is memory pressure in the system (or no activities are running and a readback completes). This memory gets lazily allocated during the first readback and it'd be the size of the texture after scaling. Freeing it is harmless at a slight reallocation cost when the next readback is triggered. BUG=641962 TBR=boliu@chromium.org ========== to ========== Android: Free GLHelper context ashmem when it makes sense Free the 'mapped_memory' that is used in readbacks through CopyFromCompositingSurface() when there is memory pressure in the system (or no activities are running and a readback completes). This memory gets lazily allocated during the first readback and it'd be the size of the texture after scaling. Freeing it is harmless at a slight reallocation cost when the next readback is triggered. BUG=641962 TBR=boliu@chromium.org Committed: https://crrev.com/98f7c133cdef22c26922c56371f84989f7a2cd60 Cr-Commit-Position: refs/heads/master@{#421357} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/98f7c133cdef22c26922c56371f84989f7a2cd60 Cr-Commit-Position: refs/heads/master@{#421357}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2392623003/ by boliu@chromium.org. The reason for reverting is: Causes a null pointer crash. And also apparently doesn't work? BUG=652050. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
