76030 - cflow optimizations. Part 2 fix - share counters and stacks when we can.

org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/AjCompilerAdapter.java

@@ -25,6 +25,7 @@ import org.aspectj.bridge.IMessageHandler;
 import org.aspectj.bridge.IProgressListener;
 import org.aspectj.weaver.bcel.BcelWeaver;
 import org.aspectj.weaver.bcel.BcelWorld;
+import org.aspectj.weaver.patterns.CflowPointcut;
 import org.aspectj.org.eclipse.jdt.internal.compiler.CompilationResult;
 import org.aspectj.org.eclipse.jdt.internal.compiler.Compiler;
 import org.aspectj.org.eclipse.jdt.internal.compiler.ICompilerAdapter;
@@ -235,8 +236,12 @@ public class AjCompilerAdapter implements ICompilerAdapter {
 //			addAllKnownClassesToWeaveList();
 //			resultsPendingWeave.addAll(getBinarySourcesFrom(binarySourceSetForFullWeave));
 //		}
-
-		weaver.weave(new WeaverAdapter(this,weaverMessageHandler,progressListener));
+		try {
+		  weaver.weave(new WeaverAdapter(this,weaverMessageHandler,progressListener));
+		} finally {
+			// ???: is this the right point for this? After weaving has finished clear the caches.
+			CflowPointcut.clearCaches();
+		}
 	}
 	
 	private void addAllKnownClassesToWeaveList() {

tests/cflow/CounterTest01.java

@@ -4,13 +4,20 @@ import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
 
+/**
+ * In this testcase we are using cflow() with a pointcut that doesn't include state -
+ * this should be managed by our new CflowCounter rather than a CflowStack.
+ * 
+ * Because the two cflow pointcuts are identical (both are 'cflow(execution(* main(..))' it also
+ * means we can share a single counter for both of them !
+ */
 public class CounterTest01 {
 
 	public static void main(String []argv) {
 		new CounterTest01().sayMessage();
 		int ctrs = ReflectionHelper.howManyCflowCounterFields(Cflow1.aspectOf());
-		if (ctrs!=2) {
-			throw new RuntimeException("Should be two cflow counters, but found: "+ctrs);
+		if (ctrs!=1) {
+			throw new RuntimeException("Should be one cflow counter, but found: "+ctrs);
 		}
 		int stacks = ReflectionHelper.howManyCflowStackFields(Cflow1.aspectOf());
 		if (stacks!=1) {

tests/cflow/CounterTest02.java

@@ -0,0 +1,85 @@
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+
+/**
+ * In this testcase we create a pointcut p1() which uses cflow and then we reference
+ * it in two other anonymous pointcuts attached to advice.  The cflow() should be managed
+ * by a counter (as no state is maintained) and the reused pointcut should not mean two
+ * counters are created.  One counter should be created and shared.
+ */
+public class CounterTest02 {
+
+	public static void main(String []argv) {
+		new CounterTest02().sayMessage();
+		int ctrs = ReflectionHelper.howManyCflowCounterFields(Cflow1.aspectOf());
+		int stacks = ReflectionHelper.howManyCflowStackFields(Cflow1.aspectOf());
+		if (ctrs!=1) 
+			throw new RuntimeException("Should be one cflow counter, but found: "+ctrs);
+		if (stacks!=1) 
+			throw new RuntimeException("Should be one cflow stacks, but found: "+stacks);
+		if (Cflow1.stackAdvice!=2) 
+			throw new RuntimeException("Expected two piece of stack advice to run: "+Cflow1.stackAdvice);
+		if (Cflow1.counterAdvice!=4) 
+			throw new RuntimeException("Expected four pieces of counter advice to run: "+Cflow1.counterAdvice);
+		
+	}
+	
+	public void sayMessage() {
+		printmsg("Hello "); printmsg("World\n");
+	}
+	
+	public void printmsg(String msg) {
+		System.out.print(msg);
+	}
+}
+
+aspect Cflow1 {
+	public static int stackAdvice = 0;
+	public static int counterAdvice = 0;
+	
+	// CflowCounter created for this pointcut should be shared below!
+	pointcut p1(): cflow(execution(* main(..)));
+	
+	before(): call(* print(..)) && p1() {
+		// Managed by a CflowCounter
+		Cflow1.counterAdvice++;
+	}
+	
+	before(): call(* print(..)) && p1() {
+		// Managed by a CflowCounter
+		Cflow1.counterAdvice++;
+	}
+	
+	before(Object o): call(* print(..)) && cflow(execution(* main(..)) && args(o)) {
+		// Managed by a CflowStack - since state is exposed
+		Cflow1.stackAdvice++;
+	}
+}
+
+class ReflectionHelper {
+  public static List getCflowfields(Object o,boolean includeCounters,boolean includeStacks) {
+  	List res = new ArrayList();
+  	Class clazz = o.getClass();
+  	Field[] fs = clazz.getDeclaredFields();
+  	for (int i = 0; i < fs.length; i++) {
+		Field f = fs[i];
+		if ((f.getType().getName().endsWith("CFlowCounter") && includeCounters) ||
+			(f.getType().getName().endsWith("CFlowStack") && includeStacks)) {
+			res.add(f.getType().getName()+":"+f.getName());
+		}
+	}
+  	return res;
+  }
+  
+  public static int howManyCflowCounterFields(Object o) {
+    return getCflowfields(o,true,false).size();
+  }
+  
+  public static int howManyCflowStackFields(Object o) {
+    return getCflowfields(o,false,true).size();
+  }
+  
+}

tests/cflow/CounterTest03.java

@@ -0,0 +1,75 @@
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+
+/**
+ * In this test, a cflow() pointcut is named and then reused.  It refers to state and so
+ * we must manage it with a CFlowStack - can we share the stacks?
+ */
+public class CounterTest03 {
+
+	public static void main(String []argv) {
+		new CounterTest03().sayMessage();
+		int ctrs = ReflectionHelper.howManyCflowCounterFields(Cflow1.aspectOf());
+		if (ctrs!=0) {
+			throw new RuntimeException("Should be zero cflow counters, but found: "+ctrs);
+		}
+		int stacks = ReflectionHelper.howManyCflowStackFields(Cflow1.aspectOf());
+		if (stacks!=1) {
+			throw new RuntimeException("Should be one cflow stacks, but found: "+stacks);
+		}
+	}
+	
+	public void sayMessage() {
+		printmsg("Hello "); printmsg("World\n");
+	}
+	
+	public void printmsg(String msg) {
+		System.out.print(msg);
+	}
+}
+
+aspect Cflow1 {
+	
+	// CflowCounter created for this pointcut should be shared below!
+	pointcut p1(Object o): cflow(execution(* main(..)) && args(o));
+	
+	before(Object o): call(* print(..)) && p1(o) {
+		// Managed by a CflowCounter
+	}
+	
+	before(Object o): call(* print(..)) && p1(o) {
+		// Managed by a CflowCounter
+	}
+	
+//	before(Object o): execution(* print(..)) && cflow(execution(* main(..)) && target(o)) {
+//		// Managed by a CflowStack - since state is exposed
+//	}
+}
+
+class ReflectionHelper {
+  public static List getCflowfields(Object o,boolean includeCounters,boolean includeStacks) {
+  	List res = new ArrayList();
+  	Class clazz = o.getClass();
+  	Field[] fs = clazz.getDeclaredFields();
+  	for (int i = 0; i < fs.length; i++) {
+		Field f = fs[i];
+		if ((f.getType().getName().endsWith("CFlowCounter") && includeCounters) ||
+			(f.getType().getName().endsWith("CFlowStack") && includeStacks)) {
+			res.add(f.getType().getName()+":"+f.getName());
+		}
+	}
+  	return res;
+  }
+  
+  public static int howManyCflowCounterFields(Object o) {
+    return getCflowfields(o,true,false).size();
+  }
+  
+  public static int howManyCflowStackFields(Object o) {
+    return getCflowfields(o,false,true).size();
+  }
+  
+}

tests/cflow/CounterTest04.java

@@ -0,0 +1,76 @@
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+
+/**
+ * In this test, we have multiple identical cflow() pointcut 'pieces' used in a number of
+ * pointcuts.  We are not naming a cflow() and reusing it, we are just duplicating the
+ * pointcut in multiple places - can we share counters?
+ */
+public class CounterTest04 {
+
+	public static void main(String []argv) {
+		new CounterTest04().sayMessage();
+		int ctrs = ReflectionHelper.howManyCflowCounterFields(Cflow1.aspectOf());
+//		if (ctrs!=2) {
+//			throw new RuntimeException("Should be two cflow counters, but found: "+ctrs);
+//		}
+		int stacks = ReflectionHelper.howManyCflowStackFields(Cflow1.aspectOf());
+		if (stacks!=2) {
+			throw new RuntimeException("Should be two cflow stacks, but found: "+stacks);
+		}
+	}
+	
+	public void sayMessage() {
+		printmsg("Hello "); printmsg("World\n");
+	}
+	
+	public void printmsg(String msg) {
+		System.out.print(msg);
+	}
+}
+
+aspect Cflow1 {
+	
+	// CflowCounter created for this pointcut should be shared below!
+	pointcut p1(Object o): cflow(execution(* main(..)) && args(o));
+	
+	before(Object o): call(* print(..)) && p1(o) {
+		// Managed by a CflowCounter
+	}
+	
+	before(Object o): call(* print(..)) && p1(o) {
+		// Managed by a CflowCounter
+	}
+	
+	before(Object o): execution(* print(..)) && cflow(execution(* main(..)) && target(o)) {
+		// Managed by a CflowStack - since state is exposed
+	}
+}
+
+class ReflectionHelper {
+  public static List getCflowfields(Object o,boolean includeCounters,boolean includeStacks) {
+  	List res = new ArrayList();
+  	Class clazz = o.getClass();
+  	Field[] fs = clazz.getDeclaredFields();
+  	for (int i = 0; i < fs.length; i++) {
+		Field f = fs[i];
+		if ((f.getType().getName().endsWith("CFlowCounter") && includeCounters) ||
+			(f.getType().getName().endsWith("CFlowStack") && includeStacks)) {
+			res.add(f.getType().getName()+":"+f.getName());
+		}
+	}
+  	return res;
+  }
+  
+  public static int howManyCflowCounterFields(Object o) {
+    return getCflowfields(o,true,false).size();
+  }
+  
+  public static int howManyCflowStackFields(Object o) {
+    return getCflowfields(o,false,true).size();
+  }
+  
+}

tests/cflow/CounterTest05.java

@@ -0,0 +1,51 @@
+public class CounterTest05 {
+
+/*
+ * Here we have an abstract pointcut that is used within a cflow.  In the two concrete sub-aspects
+ * we make the abstract pointcut concrete.  The aim of the test is to ensure we do not share
+ * the cflow counter objects, since the pointcut within the cflow() in each case points at a 
+ * different 'entry' point.  The count should be 10 when we finish.  If it is 8 we have shared
+ * a counter.
+ */
+  public static void main(String []argv) {
+    print();
+    print();
+    below1();
+    System.err.println("ctr="+A.ctr);
+    if (A.ctr!=10) 
+    	throw new RuntimeException("Counter should be 10 but is "+A.ctr);
+  }
+
+  public static void below1() {
+    print();
+    print();
+    below2();
+  }
+
+  public static void below2() {
+    print();
+    print();
+  }
+
+public static void print() {}
+}
+
+abstract aspect A {
+  public static int ctr = 0;
+
+  abstract pointcut abs();
+
+  pointcut p(): call(* print(..)) && cflow(abs());
+
+  before(): p() {
+    A.ctr++;
+  }
+}
+
+aspect B extends A {
+  pointcut abs(): execution(* main(..)); // ctr increases by 6
+}
+
+aspect C extends A {
+  pointcut abs(): execution(* below1(..)); // ctr increases by 4
+}

tests/src/org/aspectj/systemtest/ajc121/Ajc121Tests.java

@@ -308,14 +308,30 @@ public class Ajc121Tests extends org.aspectj.testing.XMLBasedAjcTestCase {
    
   public void test057_decSoftWithSuper() {
       runTest("declare soft can cause programs with invalid exception behaviour to be generated");
-    }
+  }
 
   public void test058_npeOnTJPerror() {
     runTest("NPE on thisJoinPoint mistake");
   }
   
   public void test059_cflowOptimization_counters() {
-  	runTest("Optimization of cflow - counters");
+  	runTest("Optimization of cflow - counters (1)");
+  }
+  
+  public void test060_cflowOptimization_counters() {
+  	runTest("Optimization of cflow - shared counters (2)");
+  }
+  
+  public void test061_cflowOptimization_counters() {
+  	runTest("Optimization of cflow - shared stacks (3)");
+  }
+  
+  public void test062_cflowOptimization_counters() {
+  	runTest("Optimization of cflow - counters (4)");
+  }
+   
+  public void test063_cflowOptimization_countersWithAbstractPcuts() {
+  	runTest("Optimization of cflow - counters with abstract pointcuts (5)");
   }
 
 }

tests/src/org/aspectj/systemtest/ajc121/ajc121-tests.xml

@@ -466,10 +466,27 @@
         </compile>
     </ajc-test>
 
-	<ajc-test 
-    	dir="cflow" 
-    	pr="76030"
-		title="Optimization of cflow - counters">
+	<ajc-test dir="cflow" pr="76030" title="Optimization of cflow - counters (1)">
         <compile files="CounterTest01.java"/>
 		<run class="CounterTest01"/>
-    </ajc-test>
+    </ajc-test>
+	
+	<ajc-test dir="cflow" pr="76030" title="Optimization of cflow - shared counters (2)">
+        <compile files="CounterTest02.java"/>
+		<run class="CounterTest02"/>
+    </ajc-test>
+	
+	<ajc-test dir="cflow" pr="76030" title="Optimization of cflow - shared stacks (3)">
+        <compile files="CounterTest03.java"/>
+		<run class="CounterTest03"/>
+    </ajc-test>
+	
+	<ajc-test dir="cflow" pr="76030" title="Optimization of cflow - counters (4)">
+        <compile files="CounterTest04.java"/>
+		<run class="CounterTest04"/>
+    </ajc-test>
+	
+    <ajc-test dir="cflow" pr="76030" title="Optimization of cflow - counters with abstract pointcuts (5)">
+        <compile files="CounterTest05.java"/>
+		<run class="CounterTest05"/>
+    </ajc-test>

weaver/src/org/aspectj/weaver/bcel/BcelWeaver.java

@@ -61,6 +61,7 @@ import org.aspectj.weaver.TypeX;
 import org.aspectj.weaver.WeaverMessages;
 import org.aspectj.weaver.WeaverMetrics;
 import org.aspectj.weaver.WeaverStateInfo;
+import org.aspectj.weaver.patterns.CflowPointcut;
 import org.aspectj.weaver.patterns.DeclareParents;
 import org.aspectj.weaver.patterns.FastMatchInfo;
 
@@ -320,6 +321,7 @@ public class BcelWeaver implements IWeaver {
     public void prepareForWeave() {
     	needToReweaveWorld = false;
 
+    	CflowPointcut.clearCaches();
     	
     	// update mungers
     	for (Iterator i = addedClasses.iterator(); i.hasNext(); ) { 

weaver/src/org/aspectj/weaver/patterns/CflowPointcut.java

@@ -19,6 +19,7 @@ import java.io.IOException;
 import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Hashtable;
 import java.util.List;
 
 import org.aspectj.bridge.IMessage;
@@ -41,10 +42,13 @@ import org.aspectj.weaver.ast.Test;
 
 
 public class CflowPointcut extends Pointcut {
-	private Pointcut entry;
-	private boolean isBelow;
+	private Pointcut entry; // The pointcut inside the cflow() that represents the 'entry' point
+	private boolean isBelow;// Is this cflowbelow?
 	private int[] freeVars;
 	
+	private static Hashtable cflowFields = new Hashtable();
+	private static Hashtable cflowBelowFields = new Hashtable();
+	
 	/**
 	 * Used to indicate that we're in the context of a cflow when concretizing if's
 	 * 
@@ -56,6 +60,7 @@ public class CflowPointcut extends Pointcut {
 
 	
 	public CflowPointcut(Pointcut entry, boolean isBelow, int[] freeVars) {
+	//	System.err.println("Building cflow pointcut "+entry.toString());
 		this.entry = entry;
 		this.isBelow = isBelow;
 		this.freeVars = freeVars;
@@ -167,21 +172,47 @@ public class CflowPointcut extends Pointcut {
 
 		List innerCflowEntries = new ArrayList(xcut.getCflowEntries());
 		innerCflowEntries.removeAll(previousCflowEntries);
+
+		  
+		Object field = getCflowfield(concreteEntry);
+		
+		// Four routes of interest through this code (did I hear someone say refactor??)
+		// 1) no state in the cflow - we can use a counter *and* we have seen this pointcut
+		//    before - so use the same counter as before.
+		// 2) no state in the cflow - we can use a counter, but this is the first time
+		//    we have seen this pointcut, so build the infrastructure.
+		// 3) state in the cflow - we need to use a stack *and* we have seen this pointcut
+		//    before - so share the stack.
+		// 4) state in the cflow - we need to use a stack, but this is the first time 
+		//    we have seen this pointcut, so build the infrastructure.
 		
 		if (freeVars.length == 0) { // No state, so don't use a stack, use a counter.
-		  ResolvedMember cflowField = new ResolvedMember(Member.FIELD,concreteAspect,Modifier.STATIC | Modifier.PUBLIC | Modifier.FINAL,
+		  ResolvedMember localCflowField = null;
+
+		  // Check if we have already got a counter for this cflow pointcut
+		  if (field != null) {
+		   	localCflowField = (ResolvedMember)field; // Use the one we already have
+		   	
+		  } else {
+		  	
+		  	// Create a counter field in the aspect
+		  	localCflowField = new ResolvedMember(Member.FIELD,concreteAspect,Modifier.STATIC | Modifier.PUBLIC | Modifier.FINAL,
 		  		NameMangler.cflowCounter(xcut),TypeX.forName(NameMangler.CFLOW_COUNTER_TYPE).getSignature());
 		  
-		  // Create type munger to add field to the aspect
-		  concreteAspect.crosscuttingMembers.addTypeMunger(world.makeCflowCounterFieldAdder(cflowField));
+		    // Create type munger to add field to the aspect
+		    concreteAspect.crosscuttingMembers.addTypeMunger(world.makeCflowCounterFieldAdder(localCflowField));
 		  
-		  // Create shadow munger to push stuff onto the stack
-		  concreteAspect.crosscuttingMembers.addConcreteShadowMunger(
-		  		Advice.makeCflowEntry(world,concreteEntry,isBelow,cflowField,freeVars.length,innerCflowEntries,inAspect));
-		  return new ConcreteCflowPointcut(cflowField, null,true);
+		    // Create shadow munger to push stuff onto the stack
+		    concreteAspect.crosscuttingMembers.addConcreteShadowMunger(
+		  		Advice.makeCflowEntry(world,concreteEntry,isBelow,localCflowField,freeVars.length,innerCflowEntries,inAspect));
+		    
+		    putCflowfield(concreteEntry,localCflowField); // Remember it
+	      }
+		  return new ConcreteCflowPointcut(localCflowField, null,true);
 		} else {
-		
+
 			List slots = new ArrayList();
+		  
 			for (int i=0, len=freeVars.length; i < len; i++) {
 				int freeVar = freeVars[i];
 				
@@ -197,25 +228,51 @@ public class CflowPointcut extends Pointcut {
 					new ConcreteCflowPointcut.Slot(formalIndex, formalType, i);
 				slots.add(slot);
 			}
-			
-			ResolvedMember cflowField = new ResolvedMember(
+			ResolvedMember localCflowField = null;
+			if (field != null) {
+				localCflowField = (ResolvedMember)field;
+			} else {
+		      
+			  localCflowField = new ResolvedMember(
 				Member.FIELD, concreteAspect, Modifier.STATIC | Modifier.PUBLIC | Modifier.FINAL,
 						NameMangler.cflowStack(xcut), 
 						TypeX.forName(NameMangler.CFLOW_STACK_TYPE).getSignature());
-			//System.out.println("adding field to: " + inAspect + " field " + cflowField);
+			  //System.out.println("adding field to: " + inAspect + " field " + cflowField);
 						
-			// add field and initializer to inAspect
-			//XXX and then that info above needs to be mapped down here to help with
-			//XXX getting the exposed state right
-			concreteAspect.crosscuttingMembers.addConcreteShadowMunger(
-					Advice.makeCflowEntry(world, concreteEntry, isBelow, cflowField, freeVars.length, innerCflowEntries,inAspect));
+			  // add field and initializer to inAspect
+			  //XXX and then that info above needs to be mapped down here to help with
+			  //XXX getting the exposed state right
+			  concreteAspect.crosscuttingMembers.addConcreteShadowMunger(
+					Advice.makeCflowEntry(world, concreteEntry, isBelow, localCflowField, freeVars.length, innerCflowEntries,inAspect));
 			
-			concreteAspect.crosscuttingMembers.addTypeMunger(
-				world.makeCflowStackFieldAdder(cflowField));
-
-			return new ConcreteCflowPointcut(cflowField, slots,false);
+			  concreteAspect.crosscuttingMembers.addTypeMunger(
+				world.makeCflowStackFieldAdder(localCflowField));
+			  putCflowfield(concreteEntry,localCflowField);
+		    }
+			return new ConcreteCflowPointcut(localCflowField, slots,false);
 		}
 		
 	}
+	
+	public static void clearCaches() {
+		cflowFields.clear();
+		cflowBelowFields.clear();
+	}
+	
+	private Object getCflowfield(Pointcut pcutkey) {
+		if (isBelow) {
+			return cflowBelowFields.get(pcutkey);
+		} else {
+			return cflowFields.get(pcutkey);
+		}
+	}
+	
+	private void putCflowfield(Pointcut pcutkey,Object o) {
+		if (isBelow) {
+			cflowBelowFields.put(pcutkey,o);
+		} else {
+			cflowFields.put(pcutkey,o);
+		}
+	}
 
 }