Revert "Revert of Use the GrCacheable ID to eliminate the need for notifications to GrGpuGL when te…

Revert "Revert of Use the GrCacheable ID to eliminate the need for notifications to GrGpuGL when textures and RTs are… (https://codereview.chromium.org/376703009/)"

This reverts commit 249171e7d29b5559f3eefe9dbd437030bfad3fda.

Uses 32 bit id instead of 64. Renamed instanceID to uniqueID to match existing code.

Committed: https://skia.googlesource.com/skia/+/1c63bf6e90f160c9a0d7484dedfaf87c0aa341e9

[robertphillips, 2014-07-22 18:15:46]

Questions

https://codereview.chromium.org/414493002/diff/1/include/gpu/GrCacheable.h
File include/gpu/GrCacheable.h (right):

https://codereview.chromium.org/414493002/diff/1/include/gpu/GrCacheable.h#ne...
include/gpu/GrCacheable.h:87: GrResourceCacheEntry*   fCacheEntry;  // NULL if
not in cache
Does this need to be mutable? Can it be const?

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrBitmapTextContext.cpp
File src/gpu/GrBitmapTextContext.cpp (right):

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrBitmapTextContext....
src/gpu/GrBitmapTextContext.cpp:58: fCurrVertex = 0;
= SK_InvalidGenID; ?
Although maybe we need an SK_InvalidUniqueID.

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrBitmapTextContext.h
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrBitmapTextContext....
src/gpu/GrBitmapTextContext.h:50: SkAutoTUnref<GrEffect>      fCachedEffect;
Shouldn't this be something like: 
// This is the unique ID of the GrTexture used in the
GrCustomCoordsTextureEffect cached in 'fCachedEffect' 
uint32_t fCachedEffectTextureUniqueID;

?

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrCacheable.cpp
File src/gpu/GrCacheable.cpp (right):

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrCacheable.cpp#newc...
src/gpu/GrCacheable.cpp:11: uint32_t GrCacheable::CreateUniqueID() {
= SK_InvalidGenID;
for (maybe) clarity?

https://codereview.chromium.org/414493002/diff/1/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (left):

https://codereview.chromium.org/414493002/diff/1/src/gpu/gl/GrGpuGL.cpp#oldco...
src/gpu/gl/GrGpuGL.cpp:2534: 
How is this happening with the new system?

https://codereview.chromium.org/414493002/diff/1/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/414493002/diff/1/src/gpu/gl/GrGpuGL.cpp#newco...
src/gpu/gl/GrGpuGL.cpp:290: for (int s = 0; s <
fHWBoundTextureUniqueIDs.count(); ++s) {
SK_InvalidUniqueID ?

https://codereview.chromium.org/414493002/diff/1/src/gpu/gl/GrGpuGL.cpp#newco...
src/gpu/gl/GrGpuGL.cpp:314: if (resetBits & kRenderTarget_GrGLBackendState) {
same?

[bsalomon, 2014-07-22 18:48:45]

PTAL

https://codereview.chromium.org/414493002/diff/1/include/gpu/GrCacheable.h
File include/gpu/GrCacheable.h (right):

https://codereview.chromium.org/414493002/diff/1/include/gpu/GrCacheable.h#ne...
include/gpu/GrCacheable.h:87: GrResourceCacheEntry*   fCacheEntry;  // NULL if
not in cache
On 2014/07/22 18:15:46, robertphillips wrote:
> Does this need to be mutable? Can it be const?

nope, should be const

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrBitmapTextContext.cpp
File src/gpu/GrBitmapTextContext.cpp (right):

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrBitmapTextContext....
src/gpu/GrBitmapTextContext.cpp:58: fCurrVertex = 0;
On 2014/07/22 18:15:46, robertphillips wrote:
> = SK_InvalidGenID; ?
> Although maybe we need an SK_InvalidUniqueID.

Done.

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrBitmapTextContext.h
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrBitmapTextContext....
src/gpu/GrBitmapTextContext.h:50: SkAutoTUnref<GrEffect>      fCachedEffect;
On 2014/07/22 18:15:46, robertphillips wrote:
> Shouldn't this be something like: 
> // This is the unique ID of the GrTexture used in the
> GrCustomCoordsTextureEffect cached in 'fCachedEffect' 
> uint32_t fCachedEffectTextureUniqueID;
> 
> ?
// Used to check whether fCachedEffect is still valid.

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrCacheable.cpp
File src/gpu/GrCacheable.cpp (right):

https://codereview.chromium.org/414493002/diff/1/src/gpu/GrCacheable.cpp#newc...
src/gpu/GrCacheable.cpp:11: uint32_t GrCacheable::CreateUniqueID() {
On 2014/07/22 18:15:46, robertphillips wrote:
> = SK_InvalidGenID;
> for (maybe) clarity?

Done.

https://codereview.chromium.org/414493002/diff/1/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/414493002/diff/1/src/gpu/gl/GrGpuGL.cpp#newco...
src/gpu/gl/GrGpuGL.cpp:290: for (int s = 0; s <
fHWBoundTextureUniqueIDs.count(); ++s) {
On 2014/07/22 18:15:46, robertphillips wrote:
> SK_InvalidUniqueID ?

Done.

https://codereview.chromium.org/414493002/diff/1/src/gpu/gl/GrGpuGL.cpp#newco...
src/gpu/gl/GrGpuGL.cpp:314: if (resetBits & kRenderTarget_GrGLBackendState) {
On 2014/07/22 18:15:46, robertphillips wrote:
> same?

Done.

[robertphillips, 2014-07-22 19:27:37]

https://codereview.chromium.org/414493002/diff/20001/src/gpu/GrCacheable.cpp
File src/gpu/GrCacheable.cpp (right):

https://codereview.chromium.org/414493002/diff/20001/src/gpu/GrCacheable.cpp#...
src/gpu/GrCacheable.cpp:15: id =
static_cast<uint32_t>(sk_atomic_inc(&gCacheableID) + 1);
Is this test right?

https://codereview.chromium.org/414493002/diff/20001/src/gpu/GrCacheable.cpp#...
src/gpu/GrCacheable.cpp:16: } while (id != SK_InvalidUniqueID);
SkASSERT(SK_InvalidUniqueID != id); ?

https://codereview.chromium.org/414493002/diff/20001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/414493002/diff/20001/src/gpu/gl/GrGpuGL.cpp#n...
src/gpu/gl/GrGpuGL.cpp:1488: if (renderTarget->getUniqueID() !=
fHWBoundRenderTargetUniqueID) {
SK_InvalidUniqueID ?

[bsalomon, 2014-07-22 19:49:10]

https://codereview.chromium.org/414493002/diff/20001/src/gpu/GrCacheable.cpp
File src/gpu/GrCacheable.cpp (right):

https://codereview.chromium.org/414493002/diff/20001/src/gpu/GrCacheable.cpp#...
src/gpu/GrCacheable.cpp:15: id =
static_cast<uint32_t>(sk_atomic_inc(&gCacheableID) + 1);
On 2014/07/22 19:27:37, robertphillips wrote:
> Is this test right?

I'm losing it.

https://codereview.chromium.org/414493002/diff/20001/src/gpu/GrCacheable.cpp#...
src/gpu/GrCacheable.cpp:16: } while (id != SK_InvalidUniqueID);
With the corrected loop condition it seems unnecessary. I realize I made the
typo before that would hit the assert, but had I run the code it would have been
obvious as any thread that reaches here goes into a practically infinite loop.

https://codereview.chromium.org/414493002/diff/20001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/414493002/diff/20001/src/gpu/gl/GrGpuGL.cpp#n...
src/gpu/gl/GrGpuGL.cpp:1488: if (renderTarget->getUniqueID() !=
fHWBoundRenderTargetUniqueID) {
On 2014/07/22 19:27:37, robertphillips wrote:
> SK_InvalidUniqueID ?

Done.

[robertphillips, 2014-07-22 19:53:00]

lgtm

[bsalomon, 2014-07-22 19:55:35]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-07-22 19:56:03]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/bsalomon@google.com/414493002/40001

[commit-bot: I haz the power, 2014-07-22 20:09:50]

Change committed as 1c63bf6e90f160c9a0d7484dedfaf87c0aa341e9