From: Alexander Kriegisch <Alexander@Kriegisch.name>
Date: Sat, 7 Jan 2023 09:36:08 +0000 (+0100)
Subject: BcelTypeMunger.mungeMethodDelegate: only use 'synchronized' when necessary
X-Git-Tag: V1_9_20~61
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=c8b9568a8577d2af0f1449eec729784b0dcaa484;p=aspectj.git

BcelTypeMunger.mungeMethodDelegate: only use 'synchronized' when necessary

Relates to #198. Now, we create a delegate method body which basically
looks as follows:

public void methodOne() {
  if (this.ajc$instance$MyAspect$MyMixin == null) {
    synchronized(this) {
      if (this.ajc$instance$MyAspect$MyMixin == null) {
        this.ajc$instance$MyAspect$MyMixin = MyAspect.aspectOf().createImplementation(this);
      }
    }
  }
  this.ajc$instance$MyAspect$MyMixin.methodOne();
}

The idea for the outer null check is from @aclement, see
https://github.com/eclipse/org.aspectj/pull/205#issuecomment-1371556080.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
---

diff --git a/tests/features164/declareMixin/CaseEConcurrent.java b/tests/features164/declareMixin/CaseEConcurrent.java
index df1e13774..874f249d5 100644
--- a/tests/features164/declareMixin/CaseEConcurrent.java
+++ b/tests/features164/declareMixin/CaseEConcurrent.java
@@ -39,9 +39,11 @@ public class CaseEConcurrent {
   // the byte code, e.g. with JDK tool 'javap -v'.
   /*
   public void doSomething() {
-    synchronized (this) {
-      if (id == null)
-        id = "doing something";
+    if (id == null) {
+      synchronized (this) {
+        if (id == null)
+          id = "doing something";
+      }
     }
     callMe(id);
   }
diff --git a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelTypeMunger.java b/weaver/src/main/java/org/aspectj/weaver/bcel/BcelTypeMunger.java
index 0ecad3606..3463d444a 100644
--- a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelTypeMunger.java
+++ b/weaver/src/main/java/org/aspectj/weaver/bcel/BcelTypeMunger.java
@@ -1436,6 +1436,26 @@ public class BcelTypeMunger extends ConcreteTypeMunger {
 			InstructionList body = new InstructionList();
 			InstructionFactory fact = gen.getFactory();
 
+			// Create a delegate method body which basically looks as follows:
+			//
+			// public void methodOne() {
+			//   if (this.ajc$instance$MyAspect$MyMixin == null) {
+			//     synchronized(this) {
+			//       if (this.ajc$instance$MyAspect$MyMixin == null) {
+			//         this.ajc$instance$MyAspect$MyMixin = MyAspect.aspectOf().createImplementation(this);
+			//       }
+			//     }
+			//   }
+			//   this.ajc$instance$MyAspect$MyMixin.methodOne();
+			// }
+
+			// Before synchronising on 'this', perform a preliminary null check on the delegate field. Only if it is null,
+			// it makes sense to synchronise, then check for null again and initialise the delegate, if null.
+			body.append(InstructionConstants.ALOAD_0);
+			body.append(Utility.createGet(fact, munger.getDelegate(weaver.getLazyClassGen().getType())));
+			InstructionBranch outerIfNonNull = InstructionFactory.createBranchInstruction(Constants.IFNONNULL, null);
+			body.append(outerIfNonNull);
+
 			// Wrap delegate field initialisation in 'synchronized(this)' block - MONITORENTER (beginning of 'try')
 			body.append(InstructionConstants.ALOAD_0);
 			body.append(InstructionConstants.MONITORENTER);
@@ -1444,8 +1464,8 @@ public class BcelTypeMunger extends ConcreteTypeMunger {
 			// synchronized block, see https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-3.html#jvms-3.14.
 			InstructionHandle tryStart = body.append(InstructionConstants.ALOAD_0);
 			body.append(Utility.createGet(fact, munger.getDelegate(weaver.getLazyClassGen().getType())));
-			InstructionBranch ifNonNull = InstructionFactory.createBranchInstruction(Constants.IFNONNULL, null);
-			body.append(ifNonNull);
+			InstructionBranch innerIfNonNull = InstructionFactory.createBranchInstruction(Constants.IFNONNULL, null);
+			body.append(innerIfNonNull);
 
 			// Create and store a new instance
 			body.append(InstructionConstants.ALOAD_0); // 'this' is where we'll store the field value
@@ -1494,7 +1514,7 @@ public class BcelTypeMunger extends ConcreteTypeMunger {
 
 			// Wrap delegate field initialisation in 'synchronized(this)' block - MONITOREXIT (end of 'try')
 			InstructionHandle ifNonNullElse = body.append(InstructionConstants.ALOAD_0);
-			ifNonNull.setTarget(ifNonNullElse);
+			innerIfNonNull.setTarget(ifNonNullElse);
 			body.append(InstructionConstants.MONITOREXIT);
 
 			// There was no error in the 'synchronized(this)' block -> jump to first instruction after exception handler
@@ -1519,6 +1539,8 @@ public class BcelTypeMunger extends ConcreteTypeMunger {
 			InstructionHandle afterTryCatch = body.append(InstructionConstants.ALOAD_0);
 			// Tell 'gotoAfterTryCatch' where to find the first statement after the exception handler
 			gotoAfterTryCatch.setTarget(afterTryCatch);
+			// The outer delegate field null check should also jump to the same place, if the delegate field is not null
+			outerIfNonNull.setTarget(afterTryCatch);
 			body.append(Utility.createGet(fact, munger.getDelegate(weaver.getLazyClassGen().getType())));
 
 			// args