Rebind EGL context to flush driver caches.

Rebind EGL context to flush driver caches

Android Adreno drivers do not flush caches on glFinish, making it hard to clear up app resources when going to the background.

To work around this, we introduce a glFlushDriverCachesCHROMIUM call which does the right thing based on the driver workaround list.

On most cards, this call simply does a glFinish, which will clean up driver caches. On Adreno GPUs, this call binds a NULL surface/context then re-binds the correct surface/context.

This causes the driver's texture cache to flush, reducing our memory usage by around ~6mb.

BUG=505495, 509727
Committed: https://crrev.com/9c79932e180918336f6140236a2cdedcaed4f535
Cr-Commit-Position: refs/heads/master@{#338883}

[ericrk, 2015-07-13 21:02:57]

ericrk@chromium.org changed reviewers:
+ boliu@chromium.org, piman@chromium.org

[ericrk, 2015-07-13 21:15:34]

Please take a look at this patch and let me know if the approach seems
reasonable.

The other option is to simply apply the workaround used (rebinding the context)
on glFinish, but this seemed more controlled.

https://codereview.chromium.org/1241433003/diff/1/ui/gl/gl_gl_api_implementat...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/1241433003/diff/1/ui/gl/gl_gl_api_implementat...
ui/gl/gl_gl_api_implementation.cc:543: if (switched_contexts || !current_surface
|| !surface ||
This is a bit weird as IsCurrent(surface) always returns true if surface is
NULL, so we'd never null things (it downgrades to a check for the context being
current).

[piman, 2015-07-13 21:25:29]

https://codereview.chromium.org/1241433003/diff/1/content/browser/android/in_...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/1241433003/diff/1/content/browser/android/in_...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:261:
context_provider()->ContextGL()->FlushDriverCachesCHROMIUM();
Should we do this before SetAggressivelyFreeResources?
Otherwise:
- SetAggressivelyFreeResources finished, and freed the command buffer
- FlushDriverCachesCHROMIUM reallocates the command buffer to put the command
in, and doesn't flush - so it won't actually happen until the next Flush, and we
still hold on to the command buffer memory in the mean time.

Either that or it could even be done inside of SetAggressivelyFreeResources.


Separately, don't we want to do this also for the !webview case, i.e. the other
places we call SetAggressivelyFreeResources?

https://codereview.chromium.org/1241433003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1241433003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13094:
context_->MakeCurrent(nullptr);
How about context_->ReleaseCurrent() here, instead of adding logic to handle
MakeCurrent(nullptr) in other places?

[ericrk, 2015-07-13 21:58:17]

Patchset #2 (id:20001) has been deleted

[ericrk, 2015-07-13 21:59:47]

Patchset #2 (id:40001) has been deleted

[ericrk, 2015-07-13 22:04:12]

Patchset #2 (id:60001) has been deleted

[ericrk, 2015-07-13 22:11:11]

Thanks for the feedback!

https://codereview.chromium.org/1241433003/diff/1/content/browser/android/in_...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/1241433003/diff/1/content/browser/android/in_...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:261:
context_provider()->ContextGL()->FlushDriverCachesCHROMIUM();
On 2015/07/13 21:25:29, piman (Very slow to review) wrote:
> Should we do this before SetAggressivelyFreeResources?
> Otherwise:
> - SetAggressivelyFreeResources finished, and freed the command buffer
> - FlushDriverCachesCHROMIUM reallocates the command buffer to put the command
> in, and doesn't flush - so it won't actually happen until the next Flush, and
we
> still hold on to the command buffer memory in the mean time.
> 
> Either that or it could even be done inside of SetAggressivelyFreeResources.
> 
> 
> Separately, don't we want to do this also for the !webview case, i.e. the
other
> places we call SetAggressivelyFreeResources?

Moved this to FreeEverything.

https://codereview.chromium.org/1241433003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1241433003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13094:
context_->MakeCurrent(nullptr);
On 2015/07/13 21:25:29, piman (Very slow to review) wrote:
> How about context_->ReleaseCurrent() here, instead of adding logic to handle
> MakeCurrent(nullptr) in other places?

good point.

[ericrk, 2015-07-13 22:14:30]

Patchset #3 (id:100001) has been deleted

[piman, 2015-07-13 22:48:07]

https://codereview.chromium.org/1241433003/diff/120001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1241433003/diff/120001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:330: if
(aggressively_free_resources_) {
aggressively_free_resources_ && HaveRingBuffer()

That way, we only add the commands if we need to, and otherwise avoid recreating
the command buffer just to free it.

[piman, 2015-07-13 22:48:49]

https://codereview.chromium.org/1241433003/diff/120001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1241433003/diff/120001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:330: if
(aggressively_free_resources_) {
On 2015/07/13 22:48:07, piman (Very slow to review) wrote:
> aggressively_free_resources_ && HaveRingBuffer()

aggressively_free_resources_ && helper_->HaveRingBuffer() actually, I think.

[ericrk, 2015-07-13 23:00:16]

Patchset #4 (id:140001) has been deleted

[ericrk, 2015-07-13 23:07:52]

https://codereview.chromium.org/1241433003/diff/120001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1241433003/diff/120001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:330: if
(aggressively_free_resources_) {
On 2015/07/13 22:48:48, piman (Very slow to review) wrote:
> On 2015/07/13 22:48:07, piman (Very slow to review) wrote:
> > aggressively_free_resources_ && HaveRingBuffer()
> 
> aggressively_free_resources_ && helper_->HaveRingBuffer() actually, I think.

ah, good point

[piman, 2015-07-14 02:20:42]

lgtm

[boliu, 2015-07-14 15:18:09]

description says glFinish is called on most cards. That's missing I think?

[ericrk, 2015-07-14 18:46:24]

Patchset #5 (id:180001) has been deleted

[ericrk, 2015-07-15 01:09:49]

Patchset #5 (id:200001) has been deleted

[ericrk, 2015-07-15 01:13:19]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-07-15 01:13:20]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1241433003/#ps220001
(title: "merge")

[ericrk, 2015-07-15 01:13:27]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2015-07-15 01:13:59]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-07-15 01:14:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241433003/220001

[commit-bot: I haz the power, 2015-07-15 01:29:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-15 01:29:11]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

(exceeded global retry quota)

[ericrk, 2015-07-15 16:34:46]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-07-15 16:35:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241433003/220001

[ericrk1, 2015-07-15 17:22:00]

The CQ bit was unchecked by ericrk@google.com

[ericrk1, 2015-07-15 17:23:16]

The CQ bit was checked by ericrk@google.com

[commit-bot: I haz the power, 2015-07-15 17:24:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241433003/220001

[ericrk, 2015-07-15 17:37:23]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2015-07-15 17:44:05]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-07-15 17:46:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241433003/220001

[commit-bot: I haz the power, 2015-07-15 17:55:25]

Committed patchset #5 (id:220001)

[commit-bot: I haz the power, 2015-07-15 17:56:38]

Patchset 5 (id:??) landed as
https://crrev.com/9c79932e180918336f6140236a2cdedcaed4f535
Cr-Commit-Position: refs/heads/master@{#338883}