Philippe Verdy
2005-08-11 12:12:25 UTC
in package com.limegroup.gnutella.tigertree: (see
http://www.limewire.org/nightly/clover/results/com/limegroup/gnutella/tigertree/TigerTreeCache.html
), method public synchronized void persistCache(), line 242:
dirty = true;
Little bug here: once the cache is persisted, dirty should be set to false;
this method tests dirty at begininng to see if it needs to be persisted and
if not, returns doing nothing. So dirty is already true here, and should be
set to false.
One comment here: this persistance method is synchronized on the instace,
meaning that file write may lock the cache instance, slowing down the
uploader. Let's hope it's not called too often while LimeWire is not
running. But in practice this method takes time.
One idea would be to clone the the cache in a synchronized section, to
remove the synchronization of the method; then one would actually perform
the file stream write and close.
Another possibility, instead of cloning, is to use in-memory buffer stream,
which will probably be more efficient. So we would still use a OutputStream,
except that it would fill a memory buffer, avoiding the cloning of many
objects, using more memory than if we simply used a single resizable byte
array. (Note that buffered file stream have no well defined buffer size, so
this can still perform I/O when the internal buffer is full). This done, we
just have to write the byte array in a FileStream (no need to bufferize it).
See now line 160:
Map map = (Map)ois.readObject();
this line always fails, throwing an exception, catched on line 172 (I'm not
sure it's a good idea to catch Throwable here, a more specific exception
would be welcome).
What does this mean: the createMap() method will succeed but will always
return a new HashMap, instead of reading the map from the cache file. As the
file was successfully opened, it means it is simply empty.
You can see the impact by the error log on line 173: LOG.error("Can't read
tiger trees", t), which dumps the thrown exception.
This may be particular for the test suite we use, but I wonder if the
treecache is running correctly in actual LimeWire application.
http://www.limewire.org/nightly/clover/results/com/limegroup/gnutella/tigertree/TigerTreeCache.html
), method public synchronized void persistCache(), line 242:
dirty = true;
Little bug here: once the cache is persisted, dirty should be set to false;
this method tests dirty at begininng to see if it needs to be persisted and
if not, returns doing nothing. So dirty is already true here, and should be
set to false.
One comment here: this persistance method is synchronized on the instace,
meaning that file write may lock the cache instance, slowing down the
uploader. Let's hope it's not called too often while LimeWire is not
running. But in practice this method takes time.
One idea would be to clone the the cache in a synchronized section, to
remove the synchronization of the method; then one would actually perform
the file stream write and close.
Another possibility, instead of cloning, is to use in-memory buffer stream,
which will probably be more efficient. So we would still use a OutputStream,
except that it would fill a memory buffer, avoiding the cloning of many
objects, using more memory than if we simply used a single resizable byte
array. (Note that buffered file stream have no well defined buffer size, so
this can still perform I/O when the internal buffer is full). This done, we
just have to write the byte array in a FileStream (no need to bufferize it).
See now line 160:
Map map = (Map)ois.readObject();
this line always fails, throwing an exception, catched on line 172 (I'm not
sure it's a good idea to catch Throwable here, a more specific exception
would be welcome).
What does this mean: the createMap() method will succeed but will always
return a new HashMap, instead of reading the map from the cache file. As the
file was successfully opened, it means it is simply empty.
You can see the impact by the error log on line 173: LOG.error("Can't read
tiger trees", t), which dumps the thrown exception.
This may be particular for the test suite we use, but I wonder if the
treecache is running correctly in actual LimeWire application.