incubator-directmemory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Simone Tripodi (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (DIRECTMEMORY-70) Move Pointer.class and Pointer."statistics" into an intermediate class CacheEntry
Date Wed, 29 Feb 2012 11:47:56 GMT

    [ 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

        

Mime
View raw message