[interfacekit] Re: BBlockCache bug

I noticed the same thing about the destructor and the implementation.  I coded 
up a new
implementation last night using fMark as the delimiter between used and unused 
blocks so
it works like this:

1. The fCache member is an array of (void *)'s.  The array is fCacheSize big 
and each
element of the array points to a block of memory fBlockSize long.

2. The elements in the array are ordered like this:
     0 ... (fMark - 1) - these pointers are currently in use
     fMark ... (fCacheSize - 1) - these pointers are currently not in use
That way, you only need fMark to know which blocks are in use and which ones 
aren't

3. To get a buffer, you just return fCache[fMark] and increment fMark.  This is 
O(1) which
is good and much better than our current implementation.

4. To return a buffer back to the cache, it searches from 0 ... (fMark - 1) to 
find the
block which is being returned.  Then it swaps the matching element with the one 
at (fMark
- 1) and decrements fMark.  Swapping the pointers is a pretty cheap operation 
but finding
the buffer in the array is O(n) which isn't the best.  I think Be's 
implementation is the
same though and I will confirm this in testing by testing large numbers of 
blocks managed.  

I have a rough implementation of the above and a couple of simple tests so far. 
 I will
try to finish off the tests and compare all three implementations (Be's, our 
existing one
and the one proposed above) to see which we should go with.


On Aug 27, Ingo Weinhold <bonefish@xxxxxxxxxxxxxxx> wrote:
> 
> 
> On 2003-08-27 at 10:43:40 [+0200], burton666@xxxxxxxxx wrote:
> > About BBlockCache ... I guess I caught the destructor bug:
> > 
> > BBlockCache::~BBlockCache()
> > {
> >   delete[] (int8*)fCache;
> > }
> > 
> > 
> > since fCache is allocated as
> > 
> > fCache = (void *)new _Block[CacheSize];
> > 
> > it should be deleted as
> > 
> > delete[] (_Block *)fCache;
> > 
> > 
> > I can't fix it now since I am on a windows machine (at work :P), if no one 
> > is faster I see if I can fix it within this week.
> 
> I committed the fix.
> 
> > A more important note: The current BBlockCache implementation just use 
> > malloc or new... shouldn't it use a proper memory allocator?
> 
> I think it can't without breaking compatibility. The class is documented to 
> use alloc()/new, so we can't change this.
> 
> > Yeah, I know the last BlockCache commit shows my name, but it was just a 
> > cleanup. The original implementation was by Massimiliano Origgi.
> 
> I guess, that's why his name is listed as author in the file. ;-P
> 
> Anyway, what troubles me much more is, that I had a quick glance at the 
> implementation, and I'm not very happy with it. Aside from that the 
> destructor doesn't free the unused blocks now, it's more 
> complicated/inefficient than it need be.
> 
> For one I don't see the reason for this casting orgy -- fCache is void* 
> instead of _Block*, as it should be -- but I actually don't even see why to 
> use the _Block structures at all. In Be's header fCache is void**, that is 
> it is an array with pointers to the blocks (the actual chunks of memory). 
> The fMark member should certainly be the number of used blocks, or, in other 
> words, the separation mark for used and unused block, given that one keeps 
> the used blocks at the beginning and the free ones at the end (simply done 
> by Get()ing blocks at fCache[fMark] and swapping the pointer of a Save()d 
> block with the last used one).
> 
> Furthermore I would recommend to check whether locking was successful. For 
> benaphore style lockers, things don't work too well when operating on an 
> already destroyed object, since they first write to freed memory and ask 
> questions later, but it would give a bit more safety anyway.
> 
> CU, Ingo
> 
> PS: The class isn't in the build anyway.
> 
>

Other related posts: