[
https://issues.apache.org/jira/browse/DIRECTMEMORY-70?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13219124#comment-13219124
]
Simone Tripodi commented on DIRECTMEMORY-70:
--------------------------------------------
Salut Benoit,
I just had a look at the latest patch, looks good and I don't see any reason to
hold on and keeping uncommitted, so if there is no objection shown by anybody,
please do! :)
Just few minor questions:
{code}
+public class SlabByteBufferAllocatorImpl
...
+ private final TreeMap<Integer, FixedSizeByteBufferAllocatorImpl> slabs =
new TreeMap<Integer, FixedSizeByteBufferAllocatorImpl>();
{code}
this is smart indeed and doesn't require sort operations when needed, anyway I
would suggest you to have a look at the
[ConcurrentSkipListMap|http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/ConcurrentSkipListMap.html]
not sure how concurrency is required here, didn't investigate so deep, but
anyway the SkipList has the advantage respect to the Binary tree of performing
the "get me the next" in O(1) against O(log n) of the TreeMap.
Same for {{{MergingByteBufferAllocatorImpl}}}
{code}
+ @Override
+ public int getCapacity()
+ {
+ int totalSize = 0;
+ for (final Map.Entry<Integer, FixedSizeByteBufferAllocatorImpl> entry
: slabs.entrySet())
+ {
+ totalSize += entry.getValue().getCapacity();
+ }
+ return totalSize;
+ }
{code}
this operation has a non trivial O(n) cost - I'd expect off-heap cache will
store fantazillions of elements - can't we switch over keeping an accumulator
class member, to calculate the size every time an element is put/deleted?
{code}
+ private static class LinkedByteBuffer
{code}
NICE, I like it! :)
{code}
+public class FixedSizeByteBufferAllocatorImpl
...
+ private final Queue<ByteBuffer> freeBuffers = new
ConcurrentLinkedQueue<ByteBuffer>();
{code}
IIUC here you just mind the order of queuing, right?
{code}
+ Preconditions.checkArgument( byteBuffer.capacity() == sliceSize );
{code}
trivial, but IMHO importing {{Preconditions.checkArgument}} statically looks
better (same thing for junit's Assert)
{code}
public class MemoryManagerServiceImpl<V>
..
+ private final List<ByteBufferAllocator> allocators = new
ArrayList<ByteBufferAllocator>();
{code}
I'd move to a {{{LinkedList}}} as concrete implementation, because, always
under the conditions that an OffHeap cache should contain zillions of elements,
ArrayList is subjected to be often resized (operation that has a non trivial
cost, at runtime)
Just to remark this, in order to increase performances, we have to understand
how to get rid of the Guava iterators I used to replace JOQL, they are not
efficient:
{code}
+ Iterable<Pointer<V>> result = from( new Comparator<Pointer<V>>()
+ {
+
+ public int compare( Pointer<V> o1, Pointer<V> o2 )
+ {
+ float f1 = o1.getFrequency();
+ float f2 = o2.getFrequency();
+
+ return Float.compare( f1, f2 );
+ }
+
+ } ).sortedCopy( limit( filter( pointers, new Predicate<Pointer<V>>()
+ {
+
+ @Override
+ public boolean apply( Pointer<V> input )
+ {
+ return !input.isFree();
+ }
+
+ } ), limit ) );
+
+ free( result );
{code}
if the {{{pointers}}} could be replaced by a {{{NavigableMap}}} implementation
we can save the cost of sorting, and the {{{free}}} method should be invoked as
soon as an element in the data structure matches, not collecting first and free
them in a second time (wich has the iteration cost)
+1 to apply the patch, to your discretion following my suggestions or not. Well
done and congrats, good job! :)
> Move Pointer.class and Pointer."statistics" into an intermediate class
> CacheEntry
> ---------------------------------------------------------------------------------
>
> Key: DIRECTMEMORY-70
> URL: https://issues.apache.org/jira/browse/DIRECTMEMORY-70
> Project: Apache DirectMemory
> Issue Type: Improvement
> Reporter: Benoit Perroud
> Attachments:
> DIRECTMEMORY-70-MemoryManagerService-to-hide-implementation.patch,
> DIRECTMEMORY-70-byte-bufer-allocator.patch,
> DIRECTMEMORY-70-byte-bufer-allocator.patch,
> DIRECTMEMORY-72-byteBufferAllocator-v2.patch
>
>
> In order to achieve better separation of responsabilities, attributes like
> Pointer.class, Pointer.hits, Pointer.lastHit, etc. should be moved into an
> intermediate class, for instance CacheEntry.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
