You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directmemory.apache.org by Maurizio Cucchiara <ma...@gmail.com> on 2012/01/05 23:37:04 UTC
Re: svn commit: r1227854 - in /incubator/directmemory/trunk/directmemory-cache/src:
main/java/org/apache/directmemory/memory/ test/java/org/apache/directmemory/memory/test/
I'm sorry but I have to ask you to uniform your code with the code style
convention.
I know that your patch has probably been created before we reach the
consensus.
Sent from my mobile device, so please excuse typos and brevity.
Maurizio Cucchiara
Il giorno 05/gen/2012 22.46, <bp...@apache.org> ha scritto:
> Author: bperroud
> Date: Thu Jan 5 21:46:00 2012
> New Revision: 1227854
>
> URL: http://svn.apache.org/viewvc?rev=1227854&view=rev
> Log:
> DIRECTMEMORY-55 : OffHeapMemoryBuffer leaks 1 byte at every allocation,
> firstmatch to check correct pointer size, return null when allocating on
> multiple buffers, report correct used size
>
> Added:
>
> incubator/directmemory/trunk/directmemory-cache/src/test/java/org/apache/directmemory/memory/test/MemoryManagerServiceImplTest.java
> Modified:
>
> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/OffHeapMemoryBuffer.java
>
> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/Pointer.java
>
> Modified:
> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/OffHeapMemoryBuffer.java
> URL:
> http://svn.apache.org/viewvc/incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/OffHeapMemoryBuffer.java?rev=1227854&r1=1227853&r2=1227854&view=diff
>
> ==============================================================================
> ---
> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/OffHeapMemoryBuffer.java
> (original)
> +++
> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/OffHeapMemoryBuffer.java
> Thu Jan 5 21:46:00 2012
> @@ -99,9 +99,9 @@ public class OffHeapMemoryBuffer
> Pointer fresh = new Pointer();
> fresh.bufferNumber = existing.bufferNumber;
> fresh.start = existing.start;
> - fresh.end = fresh.start + capacity;
> + fresh.end = fresh.start + capacity - 1; // 0 indexed
> fresh.free = true;
> - existing.start += capacity + 1;
> + existing.start = fresh.end + 1; // more readable
> return fresh;
> }
>
> @@ -110,7 +110,7 @@ public class OffHeapMemoryBuffer
> {
> for ( Pointer ptr : pointers )
> {
> - if ( ptr.free && ptr.end >= capacity )
> + if (ptr.free && ptr.getCapacity() >= capacity)
> {
> return ptr;
> }
> @@ -143,7 +143,7 @@ public class OffHeapMemoryBuffer
> }
> }
>
> - final byte[] swp = new byte[pointer.end - pointer.start];
> + final byte[] swp = new byte[pointer.getCapacity()];
> buf.get( swp );
> return swp;
> }
> @@ -158,8 +158,8 @@ public class OffHeapMemoryBuffer
> pointer2free.expiresIn = 0;
> pointer2free.clazz = null;
> pointer2free.directBuffer = null;
> - used.addAndGet( -( pointer2free.end - pointer2free.start ) );
> - return pointer2free.end - pointer2free.start;
> + used.addAndGet( - pointer2free.getCapacity() );
> + return pointer2free.getCapacity();
> }
>
> public void clear()
> @@ -188,8 +188,7 @@ public class OffHeapMemoryBuffer
> if ( goodOne == null )
> {
> allocationErrors++;
> - throw new NullPointerException(
> - "did not find a suitable buffer " + allocationErrors + "
> times since last cleanup" );
> + return null;
> }
>
> Pointer fresh = slice( goodOne, payload.length );
> @@ -353,8 +352,7 @@ public class OffHeapMemoryBuffer
> if ( goodOne == null )
> {
> allocationErrors++;
> - throw new NullPointerException(
> - "did not find a suitable buffer " + allocationErrors + "
> times since last cleanup" );
> + return null;
> }
>
> Pointer fresh = slice( goodOne, size );
>
> Modified:
> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/Pointer.java
> URL:
> http://svn.apache.org/viewvc/incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/Pointer.java?rev=1227854&r1=1227853&r2=1227854&view=diff
>
> ==============================================================================
> ---
> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/Pointer.java
> (original)
> +++
> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/Pointer.java
> Thu Jan 5 21:46:00 2012
> @@ -45,6 +45,16 @@ public class Pointer
>
> public ByteBuffer directBuffer = null;
>
> + public Pointer()
> + {
> + }
> +
> + public Pointer( int start, int end )
> + {
> + this.start = start;
> + this.end = end;
> + }
> +
> public byte[] content()
> {
> return null;
> @@ -66,4 +76,15 @@ public class Pointer
> {
> return (float) ( System.currentTimeMillis() - created ) / hits;
> }
> +
> + public int getCapacity()
> + {
> + return end - start + 1;
> + }
> +
> + @Override
> + public String toString()
> + {
> + return getClass().getSimpleName() + "[" + start + "," + end + "]"
> + ( free ? "" : "not" ) + "free";
> + }
> }
>
> Added:
> incubator/directmemory/trunk/directmemory-cache/src/test/java/org/apache/directmemory/memory/test/MemoryManagerServiceImplTest.java
> URL:
> http://svn.apache.org/viewvc/incubator/directmemory/trunk/directmemory-cache/src/test/java/org/apache/directmemory/memory/test/MemoryManagerServiceImplTest.java?rev=1227854&view=auto
>
> ==============================================================================
> ---
> incubator/directmemory/trunk/directmemory-cache/src/test/java/org/apache/directmemory/memory/test/MemoryManagerServiceImplTest.java
> (added)
> +++
> incubator/directmemory/trunk/directmemory-cache/src/test/java/org/apache/directmemory/memory/test/MemoryManagerServiceImplTest.java
> Thu Jan 5 21:46:00 2012
> @@ -0,0 +1,125 @@
> +package org.apache.directmemory.memory.test;
> +
> +/*
> + * Licensed to the Apache Software Foundation (ASF) under one
> + * or more contributor license agreements. See the NOTICE file
> + * distributed with this work for additional information
> + * regarding copyright ownership. The ASF licenses this file
> + * to you under the Apache License, Version 2.0 (the
> + * "License"); you may not use this file except in compliance
> + * with the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing,
> + * software distributed under the License is distributed on an
> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> + * KIND, either express or implied. See the License for the
> + * specific language governing permissions and limitations
> + * under the License.
> + */
> +
> +import junit.framework.Assert;
> +
> +import org.apache.directmemory.memory.MemoryManagerServiceImpl;
> +import org.apache.directmemory.memory.Pointer;
> +import org.junit.Test;
> +
> +public class MemoryManagerServiceImplTest {
> +
> + private static final byte[] SMALL_PAYLOAD = "ABCD".getBytes();
> +
> + @Test
> + public void testFirstMatchBorderCase() {
> +
> + // Storing a first payload of 4 bytes, 1 byte remaining in
> the buffer. When storing a second 4 bytes payload, an
> BufferOverflowException is thrown instead of an easy check.
> +
> + final int BUFFER_SIZE = 5;
> +
> + final MemoryManagerServiceImpl memoryManagerService = new
> MemoryManagerServiceImpl();
> +
> + memoryManagerService.init(1, BUFFER_SIZE);
> +
> + Pointer pointer1 =
> memoryManagerService.store(SMALL_PAYLOAD);
> + Assert.assertNotNull(pointer1);
> +
> + Pointer pointer2 =
> memoryManagerService.store(SMALL_PAYLOAD);
> + Assert.assertNull(pointer2);
> +
> + }
> +
> + @Test
> + public void testAllocateMultipleBuffers() {
> +
> + // Initializing 4 buffers of 4 bytes, MemoryManagerService
> should search for available space in another buffer.
> +
> + final int NUMBER_OF_OBJECTS = 4;
> +
> + final MemoryManagerServiceImpl memoryManagerService = new
> MemoryManagerServiceImpl();
> +
> + memoryManagerService.init(NUMBER_OF_OBJECTS,
> SMALL_PAYLOAD.length);
> +
> + for (int i = 0; i < NUMBER_OF_OBJECTS; i++) {
> + Pointer pointer =
> memoryManagerService.store(SMALL_PAYLOAD);
> + Assert.assertNotNull(pointer);
> + }
> +
> + Pointer pointerNull =
> memoryManagerService.store(SMALL_PAYLOAD);
> + Assert.assertNull(pointerNull);
> + }
> +
> +
> + @Test
> + public void testByteLeaking() {
> +
> + // Initializing 1 buffer of 10*4 bytes, should be able to
> allocate 10 objects of 4 bytes.
> +
> + final int NUMBER_OF_OBJECTS = 10;
> +
> + final MemoryManagerServiceImpl memoryManagerService = new
> MemoryManagerServiceImpl();
> + memoryManagerService.init(1, NUMBER_OF_OBJECTS *
> SMALL_PAYLOAD.length);
> +
> + for (int i = 0; i < NUMBER_OF_OBJECTS; i++) {
> + Pointer pointer =
> memoryManagerService.store(SMALL_PAYLOAD);
> + Assert.assertNotNull(pointer);
> + }
> +
> + Pointer pointerNull =
> memoryManagerService.store(SMALL_PAYLOAD);
> + Assert.assertNull(pointerNull);
> + }
> +
> +
> + @Test
> + public void testReportCorrectUsedMemory() {
> +
> + // Initializing 1 buffer of 4*4 bytes, storing and freeing
> and storing again should report correct numbers.
> +
> + final int NUMBER_OF_OBJECTS = 4;
> + final int BUFFER_SIZE = NUMBER_OF_OBJECTS *
> SMALL_PAYLOAD.length;
> +
> + final MemoryManagerServiceImpl memoryManagerService = new
> MemoryManagerServiceImpl();
> +
> + memoryManagerService.init(1, BUFFER_SIZE);
> +
> + Pointer lastPointer = null;
> + for (int i = 0; i < NUMBER_OF_OBJECTS; i++) {
> + Pointer pointer =
> memoryManagerService.store(SMALL_PAYLOAD);
> + Assert.assertNotNull(pointer);
> + lastPointer = pointer;
> + }
> +
> + // Buffer is fully used.
> + Assert.assertEquals(BUFFER_SIZE,
> memoryManagerService.getBuffers().get(0).used());
> +
> + Assert.assertNotNull(lastPointer);
> + memoryManagerService.free( lastPointer );
> +
> + Pointer pointerNotNull =
> memoryManagerService.store(SMALL_PAYLOAD);
> + Assert.assertNotNull(pointerNotNull);
> +
> + // Buffer again fully used.
> + Assert.assertEquals(BUFFER_SIZE,
> memoryManagerService.getBuffers().get(0).used());
> +
> + }
> +
> +}
>
>
>
Re: svn commit: r1227854 - in /incubator/directmemory/trunk/directmemory-cache/src:
main/java/org/apache/directmemory/memory/ test/java/org/apache/directmemory/memory/test/
Posted by Benoit Perroud <be...@noisette.ch>.
Oh, sorry. Sure I will. The patch has been created before, and on my
local sources I'm a bit further, I just did a cherry pick and commited
those one. I will refactor and reformat this part. Sorry for that.
2012/1/5 Maurizio Cucchiara <ma...@gmail.com>:
> I'm sorry but I have to ask you to uniform your code with the code style
> convention.
>
> I know that your patch has probably been created before we reach the
> consensus.
>
> Sent from my mobile device, so please excuse typos and brevity.
>
> Maurizio Cucchiara
>
> Il giorno 05/gen/2012 22.46, <bp...@apache.org> ha scritto:
>
>> Author: bperroud
>> Date: Thu Jan 5 21:46:00 2012
>> New Revision: 1227854
>>
>> URL: http://svn.apache.org/viewvc?rev=1227854&view=rev
>> Log:
>> DIRECTMEMORY-55 : OffHeapMemoryBuffer leaks 1 byte at every allocation,
>> firstmatch to check correct pointer size, return null when allocating on
>> multiple buffers, report correct used size
>>
>> Added:
>>
>> incubator/directmemory/trunk/directmemory-cache/src/test/java/org/apache/directmemory/memory/test/MemoryManagerServiceImplTest.java
>> Modified:
>>
>> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/OffHeapMemoryBuffer.java
>>
>> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/Pointer.java
>>
>> Modified:
>> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/OffHeapMemoryBuffer.java
>> URL:
>> http://svn.apache.org/viewvc/incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/OffHeapMemoryBuffer.java?rev=1227854&r1=1227853&r2=1227854&view=diff
>>
>> ==============================================================================
>> ---
>> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/OffHeapMemoryBuffer.java
>> (original)
>> +++
>> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/OffHeapMemoryBuffer.java
>> Thu Jan 5 21:46:00 2012
>> @@ -99,9 +99,9 @@ public class OffHeapMemoryBuffer
>> Pointer fresh = new Pointer();
>> fresh.bufferNumber = existing.bufferNumber;
>> fresh.start = existing.start;
>> - fresh.end = fresh.start + capacity;
>> + fresh.end = fresh.start + capacity - 1; // 0 indexed
>> fresh.free = true;
>> - existing.start += capacity + 1;
>> + existing.start = fresh.end + 1; // more readable
>> return fresh;
>> }
>>
>> @@ -110,7 +110,7 @@ public class OffHeapMemoryBuffer
>> {
>> for ( Pointer ptr : pointers )
>> {
>> - if ( ptr.free && ptr.end >= capacity )
>> + if (ptr.free && ptr.getCapacity() >= capacity)
>> {
>> return ptr;
>> }
>> @@ -143,7 +143,7 @@ public class OffHeapMemoryBuffer
>> }
>> }
>>
>> - final byte[] swp = new byte[pointer.end - pointer.start];
>> + final byte[] swp = new byte[pointer.getCapacity()];
>> buf.get( swp );
>> return swp;
>> }
>> @@ -158,8 +158,8 @@ public class OffHeapMemoryBuffer
>> pointer2free.expiresIn = 0;
>> pointer2free.clazz = null;
>> pointer2free.directBuffer = null;
>> - used.addAndGet( -( pointer2free.end - pointer2free.start ) );
>> - return pointer2free.end - pointer2free.start;
>> + used.addAndGet( - pointer2free.getCapacity() );
>> + return pointer2free.getCapacity();
>> }
>>
>> public void clear()
>> @@ -188,8 +188,7 @@ public class OffHeapMemoryBuffer
>> if ( goodOne == null )
>> {
>> allocationErrors++;
>> - throw new NullPointerException(
>> - "did not find a suitable buffer " + allocationErrors + "
>> times since last cleanup" );
>> + return null;
>> }
>>
>> Pointer fresh = slice( goodOne, payload.length );
>> @@ -353,8 +352,7 @@ public class OffHeapMemoryBuffer
>> if ( goodOne == null )
>> {
>> allocationErrors++;
>> - throw new NullPointerException(
>> - "did not find a suitable buffer " + allocationErrors + "
>> times since last cleanup" );
>> + return null;
>> }
>>
>> Pointer fresh = slice( goodOne, size );
>>
>> Modified:
>> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/Pointer.java
>> URL:
>> http://svn.apache.org/viewvc/incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/Pointer.java?rev=1227854&r1=1227853&r2=1227854&view=diff
>>
>> ==============================================================================
>> ---
>> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/Pointer.java
>> (original)
>> +++
>> incubator/directmemory/trunk/directmemory-cache/src/main/java/org/apache/directmemory/memory/Pointer.java
>> Thu Jan 5 21:46:00 2012
>> @@ -45,6 +45,16 @@ public class Pointer
>>
>> public ByteBuffer directBuffer = null;
>>
>> + public Pointer()
>> + {
>> + }
>> +
>> + public Pointer( int start, int end )
>> + {
>> + this.start = start;
>> + this.end = end;
>> + }
>> +
>> public byte[] content()
>> {
>> return null;
>> @@ -66,4 +76,15 @@ public class Pointer
>> {
>> return (float) ( System.currentTimeMillis() - created ) / hits;
>> }
>> +
>> + public int getCapacity()
>> + {
>> + return end - start + 1;
>> + }
>> +
>> + @Override
>> + public String toString()
>> + {
>> + return getClass().getSimpleName() + "[" + start + "," + end + "]"
>> + ( free ? "" : "not" ) + "free";
>> + }
>> }
>>
>> Added:
>> incubator/directmemory/trunk/directmemory-cache/src/test/java/org/apache/directmemory/memory/test/MemoryManagerServiceImplTest.java
>> URL:
>> http://svn.apache.org/viewvc/incubator/directmemory/trunk/directmemory-cache/src/test/java/org/apache/directmemory/memory/test/MemoryManagerServiceImplTest.java?rev=1227854&view=auto
>>
>> ==============================================================================
>> ---
>> incubator/directmemory/trunk/directmemory-cache/src/test/java/org/apache/directmemory/memory/test/MemoryManagerServiceImplTest.java
>> (added)
>> +++
>> incubator/directmemory/trunk/directmemory-cache/src/test/java/org/apache/directmemory/memory/test/MemoryManagerServiceImplTest.java
>> Thu Jan 5 21:46:00 2012
>> @@ -0,0 +1,125 @@
>> +package org.apache.directmemory.memory.test;
>> +
>> +/*
>> + * Licensed to the Apache Software Foundation (ASF) under one
>> + * or more contributor license agreements. See the NOTICE file
>> + * distributed with this work for additional information
>> + * regarding copyright ownership. The ASF licenses this file
>> + * to you under the Apache License, Version 2.0 (the
>> + * "License"); you may not use this file except in compliance
>> + * with the License. You may obtain a copy of the License at
>> + *
>> + * http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing,
>> + * software distributed under the License is distributed on an
>> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
>> + * KIND, either express or implied. See the License for the
>> + * specific language governing permissions and limitations
>> + * under the License.
>> + */
>> +
>> +import junit.framework.Assert;
>> +
>> +import org.apache.directmemory.memory.MemoryManagerServiceImpl;
>> +import org.apache.directmemory.memory.Pointer;
>> +import org.junit.Test;
>> +
>> +public class MemoryManagerServiceImplTest {
>> +
>> + private static final byte[] SMALL_PAYLOAD = "ABCD".getBytes();
>> +
>> + @Test
>> + public void testFirstMatchBorderCase() {
>> +
>> + // Storing a first payload of 4 bytes, 1 byte remaining in
>> the buffer. When storing a second 4 bytes payload, an
>> BufferOverflowException is thrown instead of an easy check.
>> +
>> + final int BUFFER_SIZE = 5;
>> +
>> + final MemoryManagerServiceImpl memoryManagerService = new
>> MemoryManagerServiceImpl();
>> +
>> + memoryManagerService.init(1, BUFFER_SIZE);
>> +
>> + Pointer pointer1 =
>> memoryManagerService.store(SMALL_PAYLOAD);
>> + Assert.assertNotNull(pointer1);
>> +
>> + Pointer pointer2 =
>> memoryManagerService.store(SMALL_PAYLOAD);
>> + Assert.assertNull(pointer2);
>> +
>> + }
>> +
>> + @Test
>> + public void testAllocateMultipleBuffers() {
>> +
>> + // Initializing 4 buffers of 4 bytes, MemoryManagerService
>> should search for available space in another buffer.
>> +
>> + final int NUMBER_OF_OBJECTS = 4;
>> +
>> + final MemoryManagerServiceImpl memoryManagerService = new
>> MemoryManagerServiceImpl();
>> +
>> + memoryManagerService.init(NUMBER_OF_OBJECTS,
>> SMALL_PAYLOAD.length);
>> +
>> + for (int i = 0; i < NUMBER_OF_OBJECTS; i++) {
>> + Pointer pointer =
>> memoryManagerService.store(SMALL_PAYLOAD);
>> + Assert.assertNotNull(pointer);
>> + }
>> +
>> + Pointer pointerNull =
>> memoryManagerService.store(SMALL_PAYLOAD);
>> + Assert.assertNull(pointerNull);
>> + }
>> +
>> +
>> + @Test
>> + public void testByteLeaking() {
>> +
>> + // Initializing 1 buffer of 10*4 bytes, should be able to
>> allocate 10 objects of 4 bytes.
>> +
>> + final int NUMBER_OF_OBJECTS = 10;
>> +
>> + final MemoryManagerServiceImpl memoryManagerService = new
>> MemoryManagerServiceImpl();
>> + memoryManagerService.init(1, NUMBER_OF_OBJECTS *
>> SMALL_PAYLOAD.length);
>> +
>> + for (int i = 0; i < NUMBER_OF_OBJECTS; i++) {
>> + Pointer pointer =
>> memoryManagerService.store(SMALL_PAYLOAD);
>> + Assert.assertNotNull(pointer);
>> + }
>> +
>> + Pointer pointerNull =
>> memoryManagerService.store(SMALL_PAYLOAD);
>> + Assert.assertNull(pointerNull);
>> + }
>> +
>> +
>> + @Test
>> + public void testReportCorrectUsedMemory() {
>> +
>> + // Initializing 1 buffer of 4*4 bytes, storing and freeing
>> and storing again should report correct numbers.
>> +
>> + final int NUMBER_OF_OBJECTS = 4;
>> + final int BUFFER_SIZE = NUMBER_OF_OBJECTS *
>> SMALL_PAYLOAD.length;
>> +
>> + final MemoryManagerServiceImpl memoryManagerService = new
>> MemoryManagerServiceImpl();
>> +
>> + memoryManagerService.init(1, BUFFER_SIZE);
>> +
>> + Pointer lastPointer = null;
>> + for (int i = 0; i < NUMBER_OF_OBJECTS; i++) {
>> + Pointer pointer =
>> memoryManagerService.store(SMALL_PAYLOAD);
>> + Assert.assertNotNull(pointer);
>> + lastPointer = pointer;
>> + }
>> +
>> + // Buffer is fully used.
>> + Assert.assertEquals(BUFFER_SIZE,
>> memoryManagerService.getBuffers().get(0).used());
>> +
>> + Assert.assertNotNull(lastPointer);
>> + memoryManagerService.free( lastPointer );
>> +
>> + Pointer pointerNotNull =
>> memoryManagerService.store(SMALL_PAYLOAD);
>> + Assert.assertNotNull(pointerNotNull);
>> +
>> + // Buffer again fully used.
>> + Assert.assertEquals(BUFFER_SIZE,
>> memoryManagerService.getBuffers().get(0).used());
>> +
>> + }
>> +
>> +}
>>
>>
>>
--
sent from my Nokia 3210