cc/resources/tile.h - Issue 366113002: cc: Do not cleanup tiles with raster tasks.

reveman 2014/07/28 02:24:35 can you make ScopedVector<Tile> a friend class ins

sohanjg 2014/07/28 10:10:19 In ScopedVector, we would do the actual ptr deleti

danakj 2014/07/28 10:20:03 If u do want to use scopedvector can u use ScopedP

sohanjg 2014/07/28 11:34:50 Hmm..but tiles here are ref counted (RefCountedMan

danakj 2014/07/28 12:29:28 Er, okay, putting refcounted things in ScopedVecto

vmpstr 2014/07/28 15:23:14 Tile is weird this way. It's ref counted managed,

danakj 2014/07/28 15:27:13 Oh okay, that's super funky, thanks for the explan

On 2014/07/28 15:23:14, vmpstr wrote: > On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote: > > On 2014/07/28 11:34:50, sohanjg wrote: > > > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote: > > > > On 2014/07/28 10:10:19, sohanjg wrote: > > > > > On 2014/07/28 02:24:35, reveman wrote: > > > > > > can you make ScopedVector<Tile> a friend class instead? > > > > > > > > > > In ScopedVector, we would do the actual ptr deletion from > > > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making only > > > > > ScopedVector<Tile> friend will not be sufficient. > > > > > > > > If u do want to use scopedvector can u use ScopedPtrVector instead? > > > > > > Hmm..but tiles here are ref counted (RefCountedManaged), but ScopedPtrVector > > > works with scoped ptrs (it asserts for ref counted types). > > > > Er, okay, putting refcounted things in ScopedVector sounds really weird too > > then, since it takes "ownership" of them and would delete them on destruction, > > not unref them, unless I'm missing something? > > > > Generally refptrs and data structures for scoped_ptrs do not mix. We'd use STL > > data structures for refptrs, such as using std::vector. So this sounds strange > > without knowing more, but I'll leave to reveman to sort this out, maybe it > makes > > more sense here. > > > > (Note: I would like to one day replace ScopedVector with ScopedPtrVector > > essentially, getting rid of raw pointer ownership transfers with scoped_ptrs.) > > Tile is weird this way. It's ref counted managed, which means that it acts like > a ref counted object (when it lives on tilings). However, when tilings release > it (ie ref count goes to 0), it isn't deleted, it goes to TileManager::Release > as a raw pointer. It's up to TileManager to do whatever it needs to do. Right > now, it extends its life if it has a raster task until the task finishes. > Currently, we just have a vector of raw pointers that we have to make sure to > delete, but having a scoped (ptr) vector would allow us to take care of that > automatically.

Oh okay, that's super funky, thanks for the explanation. How come it doesn't put the raw pointer into a vector<scoped_refptr<Tile>> again there, which would take a ref on it and then delete it automatically? Or if that's not possible, we could wrap that raw pointer in a scoped_ptr'd object that deletes the contained raw pointer, and stick that into a ScopedPtrVector. Or we could just use a ScopedVector but my allergies to raw pointers kicked in.

reveman 2014/07/28 15:33:07 It would be nice if RefCountedManager::Release cou

On 2014/07/28 15:27:13, danakj_OOO_until_Aug_3 wrote: > On 2014/07/28 15:23:14, vmpstr wrote: > > On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote: > > > On 2014/07/28 11:34:50, sohanjg wrote: > > > > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote: > > > > > On 2014/07/28 10:10:19, sohanjg wrote: > > > > > > On 2014/07/28 02:24:35, reveman wrote: > > > > > > > can you make ScopedVector<Tile> a friend class instead? > > > > > > > > > > > > In ScopedVector, we would do the actual ptr deletion from > > > > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making only > > > > > > ScopedVector<Tile> friend will not be sufficient. > > > > > > > > > > If u do want to use scopedvector can u use ScopedPtrVector instead? > > > > > > > > Hmm..but tiles here are ref counted (RefCountedManaged), but > ScopedPtrVector > > > > works with scoped ptrs (it asserts for ref counted types). > > > > > > Er, okay, putting refcounted things in ScopedVector sounds really weird too > > > then, since it takes "ownership" of them and would delete them on > destruction, > > > not unref them, unless I'm missing something? > > > > > > Generally refptrs and data structures for scoped_ptrs do not mix. We'd use > STL > > > data structures for refptrs, such as using std::vector. So this sounds > strange > > > without knowing more, but I'll leave to reveman to sort this out, maybe it > > makes > > > more sense here. > > > > > > (Note: I would like to one day replace ScopedVector with ScopedPtrVector > > > essentially, getting rid of raw pointer ownership transfers with > scoped_ptrs.) > > > > Tile is weird this way. It's ref counted managed, which means that it acts > like > > a ref counted object (when it lives on tilings). However, when tilings release > > it (ie ref count goes to 0), it isn't deleted, it goes to TileManager::Release > > as a raw pointer. It's up to TileManager to do whatever it needs to do. Right > > now, it extends its life if it has a raster task until the task finishes. > > Currently, we just have a vector of raw pointers that we have to make sure to > > delete, but having a scoped (ptr) vector would allow us to take care of that > > automatically. > > Oh okay, that's super funky, thanks for the explanation. How come it doesn't put > the raw pointer into a vector<scoped_refptr<Tile>> again there, which would take > a ref on it and then delete it automatically? Or if that's not possible, we > could wrap that raw pointer in a scoped_ptr'd object that deletes the contained > raw pointer, and stick that into a ScopedPtrVector. Or we could just use a > ScopedVector but my allergies to raw pointers kicked in.

It would be nice if RefCountedManager::Release could instead pass a scoped_ptr to make it clear that non ref-counted ownership is passed to the manager and we could use ScopedPtrVector in the manager. Unfortunately, this doesn't work because of this DCHECK: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... We're not allowed to create a scoped_ptr<T> when T is derived from base::subtle::RefCountedBase. ScopedVector is at least better than a std::vector IMO.

vmpstr 2014/07/28 15:33:56 We can't really ref it again for a couple of reaso

On 2014/07/28 15:27:13, danakj_OOO_until_Aug_3 wrote: > On 2014/07/28 15:23:14, vmpstr wrote: > > On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote: > > > On 2014/07/28 11:34:50, sohanjg wrote: > > > > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote: > > > > > On 2014/07/28 10:10:19, sohanjg wrote: > > > > > > On 2014/07/28 02:24:35, reveman wrote: > > > > > > > can you make ScopedVector<Tile> a friend class instead? > > > > > > > > > > > > In ScopedVector, we would do the actual ptr deletion from > > > > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making only > > > > > > ScopedVector<Tile> friend will not be sufficient. > > > > > > > > > > If u do want to use scopedvector can u use ScopedPtrVector instead? > > > > > > > > Hmm..but tiles here are ref counted (RefCountedManaged), but > ScopedPtrVector > > > > works with scoped ptrs (it asserts for ref counted types). > > > > > > Er, okay, putting refcounted things in ScopedVector sounds really weird too > > > then, since it takes "ownership" of them and would delete them on > destruction, > > > not unref them, unless I'm missing something? > > > > > > Generally refptrs and data structures for scoped_ptrs do not mix. We'd use > STL > > > data structures for refptrs, such as using std::vector. So this sounds > strange > > > without knowing more, but I'll leave to reveman to sort this out, maybe it > > makes > > > more sense here. > > > > > > (Note: I would like to one day replace ScopedVector with ScopedPtrVector > > > essentially, getting rid of raw pointer ownership transfers with > scoped_ptrs.) > > > > Tile is weird this way. It's ref counted managed, which means that it acts > like > > a ref counted object (when it lives on tilings). However, when tilings release > > it (ie ref count goes to 0), it isn't deleted, it goes to TileManager::Release > > as a raw pointer. It's up to TileManager to do whatever it needs to do. Right > > now, it extends its life if it has a raster task until the task finishes. > > Currently, we just have a vector of raw pointers that we have to make sure to > > delete, but having a scoped (ptr) vector would allow us to take care of that > > automatically. > > Oh okay, that's super funky, thanks for the explanation. How come it doesn't put > the raw pointer into a vector<scoped_refptr<Tile>> again there, which would take > a ref on it and then delete it automatically? Or if that's not possible, we > could wrap that raw pointer in a scoped_ptr'd object that deletes the contained > raw pointer, and stick that into a ScopedPtrVector. Or we could just use a > ScopedVector but my allergies to raw pointers kicked in.

We can't really ref it again for a couple of reasons: base would dcheck if you ref something that went to 0 already, and it would just call Release again when it's released again. Having a simple wrapper would work, I think. Any other approach would also work, as long as we're always deleting these tiles. If ScopedVector is on its way out of the door, then maybe a wrapper is a way to go?

sohanjg 2014/07/30 11:12:56 Lets add the Add/Release and track the reference c

On 2014/07/28 15:33:56, vmpstr wrote: > On 2014/07/28 15:27:13, danakj_OOO_until_Aug_3 wrote: > > On 2014/07/28 15:23:14, vmpstr wrote: > > > On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote: > > > > On 2014/07/28 11:34:50, sohanjg wrote: > > > > > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote: > > > > > > On 2014/07/28 10:10:19, sohanjg wrote: > > > > > > > On 2014/07/28 02:24:35, reveman wrote: > > > > > > > > can you make ScopedVector<Tile> a friend class instead? > > > > > > > > > > > > > > In ScopedVector, we would do the actual ptr deletion from > > > > > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making > only > > > > > > > ScopedVector<Tile> friend will not be sufficient. > > > > > > > > > > > > If u do want to use scopedvector can u use ScopedPtrVector instead? > > > > > > > > > > Hmm..but tiles here are ref counted (RefCountedManaged), but > > ScopedPtrVector > > > > > works with scoped ptrs (it asserts for ref counted types). > > > > > > > > Er, okay, putting refcounted things in ScopedVector sounds really weird > too > > > > then, since it takes "ownership" of them and would delete them on > > destruction, > > > > not unref them, unless I'm missing something? > > > > > > > > Generally refptrs and data structures for scoped_ptrs do not mix. We'd use > > STL > > > > data structures for refptrs, such as using std::vector. So this sounds > > strange > > > > without knowing more, but I'll leave to reveman to sort this out, maybe it > > > makes > > > > more sense here. > > > > > > > > (Note: I would like to one day replace ScopedVector with ScopedPtrVector > > > > essentially, getting rid of raw pointer ownership transfers with > > scoped_ptrs.) > > > > > > Tile is weird this way. It's ref counted managed, which means that it acts > > like > > > a ref counted object (when it lives on tilings). However, when tilings > release > > > it (ie ref count goes to 0), it isn't deleted, it goes to > TileManager::Release > > > as a raw pointer. It's up to TileManager to do whatever it needs to do. > Right > > > now, it extends its life if it has a raster task until the task finishes. > > > Currently, we just have a vector of raw pointers that we have to make sure > to > > > delete, but having a scoped (ptr) vector would allow us to take care of that > > > automatically. > > > > Oh okay, that's super funky, thanks for the explanation. How come it doesn't > put > > the raw pointer into a vector<scoped_refptr<Tile>> again there, which would > take > > a ref on it and then delete it automatically? Or if that's not possible, we > > could wrap that raw pointer in a scoped_ptr'd object that deletes the > contained > > raw pointer, and stick that into a ScopedPtrVector. Or we could just use a > > ScopedVector but my allergies to raw pointers kicked in. > > We can't really ref it again for a couple of reasons: base would dcheck if you > ref something that went to 0 already, and it would just call Release again when > it's released again. Having a simple wrapper would work, I think. Any other > approach would also work, as long as we're always deleting these tiles. If > ScopedVector is on its way out of the door, then maybe a wrapper is a way to go?

Lets add the Add/Release and track the reference count and use ScopedPtrVector in TileManager in a followup CL ? this has been iterating for ever now :( danakj@, jamesr@ , hope that is ok with you.

Unified Diff: cc/resources/tile.h