commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simone Tripodi <simonetrip...@apache.org>
Subject 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
Date Sat, 03 Mar 2012 01:09:04 GMT
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 <simonetripodi@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,  <marcosperanza@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
>> +                            }
>> +                        }
>> +                    }
>> +                }
>> +
>> +            }
>> +        }
>> +    }
>> +
>>  }
>>
>>
Mime
View raw message