You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Simone Tripodi <si...@apache.org> on 2012/03/03 01:55:16 UTC
Re: svn commit: r1296530 - in /commons/sandbox/graph/trunk: pom.xml
src/main/java/org/apache/commons/graph/CommonsGraph.java src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java
> + <repositories>
> + <repository>
> + <id>opensymphony-releases</id>
> + <name>Opensymphony Releases</name>
> + <url>https://oss.sonatype.org/content/repositories/opensymphony-releases</url>
> + </repository>
> + </repositories>
not good to release the [graph] component, external repos not allowed
(and OpenSymphony dead http://www.opensymphony.com/)
> + static private class GraphInvocationHandler<T>
the `private`modifier should be put before; GraphInvocationHandler
class can be final
> + protected Object lock;
> +
> + Set<Method> synchronizedMethods;
> +
> + T checkedToBeSynchronized;
members can be final
> + <dependency>
> + <groupId>net.sourceforge.groboutils</groupId>
> + <artifactId>groboutils-core</artifactId>
> + <version>5</version>
that "violates" the original contract of not having dependencies -
maybe the commit message doesn't contain the `test` scope or it is
missing?
> + @Test
> + public final void testMultiTh()
name is not really expressive here. Test method is already marked as
@Test (so sounds a little redundant) but anyway the rest doesn't help
on understanding what the test methods performs
HTH, best,
-Simo
http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/
On Sat, Mar 3, 2012 at 1:36 AM, <ma...@apache.org> wrote:
> Author: marcosperanza
> Date: Sat Mar 3 00:36:01 2012
> New Revision: 1296530
>
> URL: http://svn.apache.org/viewvc?rev=1296530&view=rev
> Log:
> [SANDBOX-400] Switch the Base graph implementation underlying data structures to the thread safe version
>
> Modified:
> commons/sandbox/graph/trunk/pom.xml
> commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java
> commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java
>
> Modified: commons/sandbox/graph/trunk/pom.xml
> URL: http://svn.apache.org/viewvc/commons/sandbox/graph/trunk/pom.xml?rev=1296530&r1=1296529&r2=1296530&view=diff
> ==============================================================================
> --- commons/sandbox/graph/trunk/pom.xml (original)
> +++ commons/sandbox/graph/trunk/pom.xml Sat Mar 3 00:36:01 2012
> @@ -113,15 +113,28 @@
> <commons.componentid>graph</commons.componentid>
> <commons.jira.componentid>12314503</commons.jira.componentid>
> </properties>
> -
> +
> +
> <dependencies>
> <dependency>
> - <groupId>junit</groupId>
> - <artifactId>junit</artifactId>
> - <version>4.10</version>
> - <scope>test</scope>
> + <groupId>junit</groupId>
> + <artifactId>junit</artifactId>
> + <version>4.10</version>
> + <scope>test</scope>
> + </dependency>
> + <dependency>
> + <groupId>net.sourceforge.groboutils</groupId>
> + <artifactId>groboutils-core</artifactId>
> + <version>5</version>
> </dependency>
> - </dependencies>
> +</dependencies>
>
> <build>
> <resources>
>
> Modified: commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java
> URL: http://svn.apache.org/viewvc/commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java?rev=1296530&r1=1296529&r2=1296530&view=diff
> ==============================================================================
> --- commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java (original)
> +++ commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java Sat Mar 3 00:36:01 2012
> @@ -23,6 +23,7 @@ import static java.lang.reflect.Proxy.ne
> import static org.apache.commons.graph.utils.Assertions.checkNotNull;
>
> import java.lang.reflect.InvocationHandler;
> +import java.lang.reflect.InvocationTargetException;
> import java.lang.reflect.Method;
> import java.util.HashSet;
> import java.util.Set;
> @@ -291,27 +292,49 @@ public final class CommonsGraph<V extend
> {
> synchronizedMethods.add( method );
> }
> + GraphInvocationHandler<T> handler =
> + new GraphInvocationHandler<T>( synchronizedMethods, checkedToBeSynchronized );
> + Object proxy = newProxyInstance( type.getClassLoader(), new Class<?>[] { type }, handler );
> + handler.lock = proxy;
> + return type.cast( proxy );
> + }
> +
> +
> + static private class GraphInvocationHandler<T>
> + implements InvocationHandler
> + {
> + protected Object lock;
> +
> + Set<Method> synchronizedMethods;
> +
> + T checkedToBeSynchronized;
>
> - return type.cast( newProxyInstance( type.getClassLoader(), new Class<?>[] { type },
> - new InvocationHandler()
> - {
> -
> - private final Object lock = new Object();
> -
> - public Object invoke( Object proxy, Method method, Object[] args )
> - throws Throwable
> - {
> - if ( synchronizedMethods.contains( method ) )
> - {
> - synchronized ( this.lock )
> - {
> - return method.invoke( checkedToBeSynchronized, args );
> - }
> - }
> - return method.invoke( checkedToBeSynchronized, args );
> - }
> + public GraphInvocationHandler( Set<Method> synchronizedMethods, T checkedToBeSynchronized )
> + {
> + this.synchronizedMethods = synchronizedMethods;
> + this.checkedToBeSynchronized = checkedToBeSynchronized;
> + }
> +
> + public Object invoke( Object proxy, Method method, Object[] args )
> + throws Throwable
> + {
> + if ( synchronizedMethods.contains( method ) )
> + {
> + synchronized ( this.lock )
> + {
> + try
> + {
> + return method.invoke( checkedToBeSynchronized, args );
> + }
> + catch ( InvocationTargetException e )
> + {
> + throw e.getTargetException();
> + }
> + }
> + }
> + return method.invoke( checkedToBeSynchronized, args );
> + }
>
> - } ) );
> }
>
> /**
>
> Modified: commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java
> URL: http://svn.apache.org/viewvc/commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java?rev=1296530&r1=1296529&r2=1296530&view=diff
> ==============================================================================
> --- commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java (original)
> +++ commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java Sat Mar 3 00:36:01 2012
> @@ -30,7 +30,12 @@ import static org.junit.Assert.fail;
> import java.util.ArrayList;
> import java.util.List;
>
> +import net.sourceforge.groboutils.junit.v1.MultiThreadedTestRunner;
> +import net.sourceforge.groboutils.junit.v1.TestRunnable;
> +
> +import org.apache.commons.graph.CommonsGraph;
> import org.apache.commons.graph.GraphException;
> +import org.apache.commons.graph.MutableGraph;
> import org.junit.Test;
>
> /**
> @@ -367,4 +372,111 @@ public class BaseMutableGraphTestCase
> g.removeEdge( e );
>
> }
> +
> + /**
> + * Test Graph model ina multi-thread enviroment.
> + */
> + @Test
> + public final void testMultiTh()
> + throws Throwable
> + {
> + final MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g =
> + (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) CommonsGraph.synchronize( (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) new UndirectedMutableGraph<BaseLabeledVertex, BaseLabeledEdge>() );
> +
> + TestRunnable tr1, tr2, tr3;
> + tr1 = new GraphInsert( g, 0, 10 );
> + tr2 = new GraphInsert( g, 10, 20 );
> + tr3 = new GraphInsert( g, 20, 30 );
> +
> + TestRunnable[] trs = { tr1, tr2, tr3 };
> + MultiThreadedTestRunner mttr = new MultiThreadedTestRunner( trs );
> +
> + // kickstarts the MTTR & fires off threads
> + mttr.runTestRunnables();
> +
> + assertEquals( 30, g.getOrder() );
> +
> + // test the # of edges = n (n-1)/2
> + assertEquals( ( 30 * ( 30 - 1 ) / 2 ), g.getSize() );
> + }
> +
> + /**
> + * Test Graph model in a multi-thread enviroment.
> + */
> + @Test
> + public final void testDirectedMultiTh()
> + throws Throwable
> + {
> + final MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g =
> + (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) CommonsGraph.synchronize( (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) new DirectedMutableGraph<BaseLabeledVertex, BaseLabeledEdge>() );
> +
> + TestRunnable tr1, tr2, tr3;
> + tr1 = new GraphInsert( g, 0, 10 );
> + tr2 = new GraphInsert( g, 10, 20 );
> + tr3 = new GraphInsert( g, 20, 30 );
> +
> + TestRunnable[] trs = { tr1, tr2, tr3 };
> + MultiThreadedTestRunner mttr = new MultiThreadedTestRunner( trs );
> +
> + // kickstarts the MTTR & fires off threads
> + mttr.runTestRunnables();
> +
> + assertEquals( 30, g.getOrder() );
> +
> + // test the # of edges = n (n-1)
> + assertEquals( ( 30 * ( 30 - 1 ) ), g.getSize() );
> + }
> +
> + private class GraphInsert
> + extends TestRunnable
> + {
> +
> + private MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g;
> +
> + private int start;
> +
> + private int end;
> +
> + private GraphInsert( MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g, int start, int end )
> + {
> + this.g = g;
> + this.start = start;
> + this.end = end;
> + }
> +
> + public void runTest()
> + throws Throwable
> + {
> +
> + // building a complete Graph
> + for ( int i = start; i < end; i++ )
> + {
> + BaseLabeledVertex v = new BaseLabeledVertex( valueOf( i ) );
> + g.addVertex( v );
> + }
> + synchronized ( g )
> + {
> + for ( BaseLabeledVertex v1 : g.getVertices() )
> + {
> + for ( BaseLabeledVertex v2 : g.getVertices() )
> + {
> + if ( !v1.equals( v2 ) )
> + {
> + try
> + {
> + BaseLabeledEdge e = new BaseLabeledEdge( v1 + " -> " + v2 );
> + g.addEdge( v1, e, v2 );
> + }
> + catch ( GraphException e )
> + {
> + // ignore
> + }
> + }
> + }
> + }
> +
> + }
> + }
> + }
> +
> }
>
>
Re: svn commit: r1296530 - in /commons/sandbox/graph/trunk: pom.xml
src/main/java/org/apache/commons/graph/CommonsGraph.java src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java
Posted by Simone Tripodi <si...@apache.org>.
I checked out the code - honestly I didn't understand which are the
benefit of managing the lock object in that way.
Wouldn't have been enough synchronizing on `this`? It is what it does.
time to sleep, best,
-Simo
http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/
On Sat, Mar 3, 2012 at 1:55 AM, Simone Tripodi <si...@apache.org> wrote:
>> + <repositories>
>> + <repository>
>> + <id>opensymphony-releases</id>
>> + <name>Opensymphony Releases</name>
>> + <url>https://oss.sonatype.org/content/repositories/opensymphony-releases</url>
>> + </repository>
>> + </repositories>
>
> not good to release the [graph] component, external repos not allowed
> (and OpenSymphony dead http://www.opensymphony.com/)
>
>> + static private class GraphInvocationHandler<T>
>
> the `private`modifier should be put before; GraphInvocationHandler
> class can be final
>
>> + protected Object lock;
>> +
>> + Set<Method> synchronizedMethods;
>> +
>> + T checkedToBeSynchronized;
>
> members can be final
>
>> + <dependency>
>> + <groupId>net.sourceforge.groboutils</groupId>
>> + <artifactId>groboutils-core</artifactId>
>> + <version>5</version>
>
> that "violates" the original contract of not having dependencies -
> maybe the commit message doesn't contain the `test` scope or it is
> missing?
>
>> + @Test
>> + public final void testMultiTh()
>
> name is not really expressive here. Test method is already marked as
> @Test (so sounds a little redundant) but anyway the rest doesn't help
> on understanding what the test methods performs
>
> HTH, best,
> -Simo
>
> http://people.apache.org/~simonetripodi/
> http://simonetripodi.livejournal.com/
> http://twitter.com/simonetripodi
> http://www.99soft.org/
>
>
>
> On Sat, Mar 3, 2012 at 1:36 AM, <ma...@apache.org> wrote:
>> Author: marcosperanza
>> Date: Sat Mar 3 00:36:01 2012
>> New Revision: 1296530
>>
>> URL: http://svn.apache.org/viewvc?rev=1296530&view=rev
>> Log:
>> [SANDBOX-400] Switch the Base graph implementation underlying data structures to the thread safe version
>>
>> Modified:
>> commons/sandbox/graph/trunk/pom.xml
>> commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java
>> commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java
>>
>> Modified: commons/sandbox/graph/trunk/pom.xml
>> URL: http://svn.apache.org/viewvc/commons/sandbox/graph/trunk/pom.xml?rev=1296530&r1=1296529&r2=1296530&view=diff
>> ==============================================================================
>> --- commons/sandbox/graph/trunk/pom.xml (original)
>> +++ commons/sandbox/graph/trunk/pom.xml Sat Mar 3 00:36:01 2012
>> @@ -113,15 +113,28 @@
>> <commons.componentid>graph</commons.componentid>
>> <commons.jira.componentid>12314503</commons.jira.componentid>
>> </properties>
>> -
>> +
>
>> +
>> <dependencies>
>> <dependency>
>> - <groupId>junit</groupId>
>> - <artifactId>junit</artifactId>
>> - <version>4.10</version>
>> - <scope>test</scope>
>> + <groupId>junit</groupId>
>> + <artifactId>junit</artifactId>
>> + <version>4.10</version>
>> + <scope>test</scope>
>> + </dependency>
>> + <dependency>
>> + <groupId>net.sourceforge.groboutils</groupId>
>> + <artifactId>groboutils-core</artifactId>
>> + <version>5</version>
>> </dependency>
>> - </dependencies>
>> +</dependencies>
>>
>> <build>
>> <resources>
>>
>> Modified: commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java
>> URL: http://svn.apache.org/viewvc/commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java?rev=1296530&r1=1296529&r2=1296530&view=diff
>> ==============================================================================
>> --- commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java (original)
>> +++ commons/sandbox/graph/trunk/src/main/java/org/apache/commons/graph/CommonsGraph.java Sat Mar 3 00:36:01 2012
>> @@ -23,6 +23,7 @@ import static java.lang.reflect.Proxy.ne
>> import static org.apache.commons.graph.utils.Assertions.checkNotNull;
>>
>> import java.lang.reflect.InvocationHandler;
>> +import java.lang.reflect.InvocationTargetException;
>> import java.lang.reflect.Method;
>> import java.util.HashSet;
>> import java.util.Set;
>> @@ -291,27 +292,49 @@ public final class CommonsGraph<V extend
>> {
>> synchronizedMethods.add( method );
>> }
>> + GraphInvocationHandler<T> handler =
>> + new GraphInvocationHandler<T>( synchronizedMethods, checkedToBeSynchronized );
>> + Object proxy = newProxyInstance( type.getClassLoader(), new Class<?>[] { type }, handler );
>> + handler.lock = proxy;
>> + return type.cast( proxy );
>> + }
>> +
>> +
>> + static private class GraphInvocationHandler<T>
>> + implements InvocationHandler
>> + {
>> + protected Object lock;
>> +
>> + Set<Method> synchronizedMethods;
>> +
>> + T checkedToBeSynchronized;
>>
>> - return type.cast( newProxyInstance( type.getClassLoader(), new Class<?>[] { type },
>> - new InvocationHandler()
>> - {
>> -
>> - private final Object lock = new Object();
>> -
>> - public Object invoke( Object proxy, Method method, Object[] args )
>> - throws Throwable
>> - {
>> - if ( synchronizedMethods.contains( method ) )
>> - {
>> - synchronized ( this.lock )
>> - {
>> - return method.invoke( checkedToBeSynchronized, args );
>> - }
>> - }
>> - return method.invoke( checkedToBeSynchronized, args );
>> - }
>> + public GraphInvocationHandler( Set<Method> synchronizedMethods, T checkedToBeSynchronized )
>> + {
>> + this.synchronizedMethods = synchronizedMethods;
>> + this.checkedToBeSynchronized = checkedToBeSynchronized;
>> + }
>> +
>> + public Object invoke( Object proxy, Method method, Object[] args )
>> + throws Throwable
>> + {
>> + if ( synchronizedMethods.contains( method ) )
>> + {
>> + synchronized ( this.lock )
>> + {
>> + try
>> + {
>> + return method.invoke( checkedToBeSynchronized, args );
>> + }
>> + catch ( InvocationTargetException e )
>> + {
>> + throw e.getTargetException();
>> + }
>> + }
>> + }
>> + return method.invoke( checkedToBeSynchronized, args );
>> + }
>>
>> - } ) );
>> }
>>
>> /**
>>
>> Modified: commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java
>> URL: http://svn.apache.org/viewvc/commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java?rev=1296530&r1=1296529&r2=1296530&view=diff
>> ==============================================================================
>> --- commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java (original)
>> +++ commons/sandbox/graph/trunk/src/test/java/org/apache/commons/graph/model/BaseMutableGraphTestCase.java Sat Mar 3 00:36:01 2012
>> @@ -30,7 +30,12 @@ import static org.junit.Assert.fail;
>> import java.util.ArrayList;
>> import java.util.List;
>>
>> +import net.sourceforge.groboutils.junit.v1.MultiThreadedTestRunner;
>> +import net.sourceforge.groboutils.junit.v1.TestRunnable;
>> +
>> +import org.apache.commons.graph.CommonsGraph;
>> import org.apache.commons.graph.GraphException;
>> +import org.apache.commons.graph.MutableGraph;
>> import org.junit.Test;
>>
>> /**
>> @@ -367,4 +372,111 @@ public class BaseMutableGraphTestCase
>> g.removeEdge( e );
>>
>> }
>> +
>> + /**
>> + * Test Graph model ina multi-thread enviroment.
>> + */
>> + @Test
>> + public final void testMultiTh()
>> + throws Throwable
>> + {
>> + final MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g =
>> + (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) CommonsGraph.synchronize( (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) new UndirectedMutableGraph<BaseLabeledVertex, BaseLabeledEdge>() );
>> +
>> + TestRunnable tr1, tr2, tr3;
>> + tr1 = new GraphInsert( g, 0, 10 );
>> + tr2 = new GraphInsert( g, 10, 20 );
>> + tr3 = new GraphInsert( g, 20, 30 );
>> +
>> + TestRunnable[] trs = { tr1, tr2, tr3 };
>> + MultiThreadedTestRunner mttr = new MultiThreadedTestRunner( trs );
>> +
>> + // kickstarts the MTTR & fires off threads
>> + mttr.runTestRunnables();
>> +
>> + assertEquals( 30, g.getOrder() );
>> +
>> + // test the # of edges = n (n-1)/2
>> + assertEquals( ( 30 * ( 30 - 1 ) / 2 ), g.getSize() );
>> + }
>> +
>> + /**
>> + * Test Graph model in a multi-thread enviroment.
>> + */
>> + @Test
>> + public final void testDirectedMultiTh()
>> + throws Throwable
>> + {
>> + final MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g =
>> + (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) CommonsGraph.synchronize( (MutableGraph<BaseLabeledVertex, BaseLabeledEdge>) new DirectedMutableGraph<BaseLabeledVertex, BaseLabeledEdge>() );
>> +
>> + TestRunnable tr1, tr2, tr3;
>> + tr1 = new GraphInsert( g, 0, 10 );
>> + tr2 = new GraphInsert( g, 10, 20 );
>> + tr3 = new GraphInsert( g, 20, 30 );
>> +
>> + TestRunnable[] trs = { tr1, tr2, tr3 };
>> + MultiThreadedTestRunner mttr = new MultiThreadedTestRunner( trs );
>> +
>> + // kickstarts the MTTR & fires off threads
>> + mttr.runTestRunnables();
>> +
>> + assertEquals( 30, g.getOrder() );
>> +
>> + // test the # of edges = n (n-1)
>> + assertEquals( ( 30 * ( 30 - 1 ) ), g.getSize() );
>> + }
>> +
>> + private class GraphInsert
>> + extends TestRunnable
>> + {
>> +
>> + private MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g;
>> +
>> + private int start;
>> +
>> + private int end;
>> +
>> + private GraphInsert( MutableGraph<BaseLabeledVertex, BaseLabeledEdge> g, int start, int end )
>> + {
>> + this.g = g;
>> + this.start = start;
>> + this.end = end;
>> + }
>> +
>> + public void runTest()
>> + throws Throwable
>> + {
>> +
>> + // building a complete Graph
>> + for ( int i = start; i < end; i++ )
>> + {
>> + BaseLabeledVertex v = new BaseLabeledVertex( valueOf( i ) );
>> + g.addVertex( v );
>> + }
>> + synchronized ( g )
>> + {
>> + for ( BaseLabeledVertex v1 : g.getVertices() )
>> + {
>> + for ( BaseLabeledVertex v2 : g.getVertices() )
>> + {
>> + if ( !v1.equals( v2 ) )
>> + {
>> + try
>> + {
>> + BaseLabeledEdge e = new BaseLabeledEdge( v1 + " -> " + v2 );
>> + g.addEdge( v1, e, v2 );
>> + }
>> + catch ( GraphException e )
>> + {
>> + // ignore
>> + }
>> + }
>> + }
>> + }
>> +
>> + }
>> + }
>> + }
>> +
>> }
>>
>>