cc: Remove SkSurface cache from Resource

cc: Remove SkSurface cache from Resource

The SkSurface that was being cached within the Resource was being held
and counted against skia's budget.  cc's budget is 512MB, and skia's is
~100MB, so cc would always blow skia's budget.  In the case of the ugam
page, we would end up redecoding, reupload and regenerating all of the
mip maps for each of the images on every frame.

Solution is to stop caching the SkSurface.

Additionally, we should investigate if this situation should even be
counted against skia's budget.  See crbug.com/452344

BUG=452138
BUG=447291
Committed: https://crrev.com/1e20542a1645750a019d0b482754cb8c8a29c413
Cr-Commit-Position: refs/heads/master@{#313547}

[hendrikw, 2015-01-27 02:41:52]

hendrikw@chromium.org changed reviewers:
+ reveman@chromium.org, vmiura@chromium.org

[hendrikw, 2015-01-27 02:41:52]

PTAL.  I'd like to check telemetry to see if it impacts performance that much
(if at all).  The first time I tried to remove this, there was a slight
regression on a few tests.  Even if this is true, this change is still important
because it might fix several issues.

[reveman, 2015-01-27 13:10:14]

https://codereview.chromium.org/874353007/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/874353007/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:339: bool can_use_lcd_text);
Does this have to return a RefPtr? I prefer if we store the reference as a
ScopedWriteLockGr member and keep having this return a raw pointer and no
ownership of the surface. If you need to pass ownership, then I think the name
of this function should probable change. ie. CreateSkSurface.

[hendrikw, 2015-01-27 16:46:59]

On 2015/01/27 13:10:14, reveman (OOO Jan 28 - Feb 9) wrote:
>
https://codereview.chromium.org/874353007/diff/1/cc/resources/resource_provid...
> File cc/resources/resource_provider.h (right):
> 
>
https://codereview.chromium.org/874353007/diff/1/cc/resources/resource_provid...
> cc/resources/resource_provider.h:339: bool can_use_lcd_text);
> Does this have to return a RefPtr? I prefer if we store the reference as a
> ScopedWriteLockGr member and keep having this return a raw pointer and no
> ownership of the surface. If you need to pass ownership, then I think the name
> of this function should probable change. ie. CreateSkSurface.

Yeah, I see.  Since we always create the lock before getting the surface, the
function is always going to create the sksurface, so in the interest of making
it obvious what's actually happening, I'm renaming it to Create.

Also, this avoids us from having to check if the surface properties match the
required surface properties.

[hendrikw, 2015-01-27 16:50:59]

Renamed function, PTAL, thanks!

[danakj, 2015-01-27 17:29:55]

On 2015/01/27 16:50:59, hendrikw wrote:
> Renamed function, PTAL, thanks!

Sounds like Brian has an idea what might be broken in skia. Do we need this
change?

[hendrikw, 2015-01-27 17:32:36]

On 2015/01/27 17:29:55, danakj wrote:
> On 2015/01/27 16:50:59, hendrikw wrote:
> > Renamed function, PTAL, thanks!
> 
> Sounds like Brian has an idea what might be broken in skia. Do we need this
> change?

When I spoke to the skia guys a few months ago about this, they mentioned that
we shouldn't really need to cache this.  It simplifies the code, and I want to
see how it affects performance.  We could always revert if things are noticeably
bad.

[reveman, 2015-01-27 21:29:05]

I like the idea of removing the caching of surfaces if it's no longer necessary.

https://codereview.chromium.org/874353007/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/874353007/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.h:339: bool can_use_lcd_text);
Sorry for not thinking of this at first but creating the surface in the ctor
would actually be more consistent with Scoped*LockSoftware and I think a bit
more appropriate. With the current patch it's possible to create multiple
surfaces from one ScopedWriteLockGr and it's also not perfectly clear that you
shouldn't hold on to the returned ref after the LockGr has been destroyed.

I think it's much better to move these parameters to the ctor and create the
surface at that time instead.

[hendrikw, 2015-01-27 22:30:57]

PTAL, I've moved the sksurface into the lock.

[reveman, 2015-01-27 22:40:22]

lgtm

https://codereview.chromium.org/874353007/diff/40001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/874353007/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1105: bool can_use_lcd_text) {
No need to change this but I'd just move this code into the ctor and avoid
having a CreateSkSurface function all together.

[hendrikw, 2015-01-27 22:52:41]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-01-27 22:52:58]

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

[commit-bot: I haz the power, 2015-01-27 22:58:19]

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

[commit-bot: I haz the power, 2015-01-27 22:58:20]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hendrikw, 2015-01-28 17:33:36]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-01-28 17:34:25]

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

[commit-bot: I haz the power, 2015-01-28 18:44:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-28 18:45:08]

Patchset 4 (id:??) landed as
https://crrev.com/1e20542a1645750a019d0b482754cb8c8a29c413
Cr-Commit-Position: refs/heads/master@{#313547}