[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #419: Reduce performance degradation caused by commit b0f59ef7c8

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419#discussion_r488066492



##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ElementUnparser.scala
##########
@@ -477,7 +477,7 @@ sealed trait RegularElementUnparserStartEndStrategy
       }
       val cur = state.currentInfosetNode
       if (cur.isComplex)
-        cur.asComplex.setFinal()
+        cur.isFinal = true

Review comment:
       I think it's actually both a mixture of removeing the trait, and making things publicly modifable. For example, with a trait we get something like this:
   
   ```
   public class WithFinalTrait implements Finalizable {
     private boolean Finalizable$$_isFinal;
   
     public void Finalizable$$_isFinal_$eq(boolean);
       Code:
          0: aload_0
          1: iload_1
          2: putfield      #15                 // Field Finalizable$$_isFinal:Z
          5: return
   
     public void setFinal();
       Code:
          0: aload_0
          1: invokestatic  #27                 // Method Finalizable$class.setFinal:(LFinalizable;)V
          4: return
   
   }
   
   public abstract class Finalizable$class {
     public static void setFinal(Finalizable);
       Code:
          0: aload_0
          1: iconst_1
          2: invokeinterface #13,  2           // InterfaceMethod Finalizable.Finalizable$$_isFinal_$eq:(Z)V
          7: return
   }
   
   ```
   
   So to set the flag we first call setFinal, which calls the static setFinal method on Finalizable, which then calls \_isFinal_$eq, which finaly sets the variable to set the value. So three function calls: setFinal, static setFinal, and isFinal_$eq.
   
   Without the trait, but with private _isFinal, the bytecode is this:
   
   public class WithOutFinalTrait {
     private boolean _isFinal;
   
     private void _isFinal_$eq(boolean);
       Code:
          0: aload_0
          1: iload_1
          2: putfield      #13                 // Field _isFinal:Z
          5: return
   
     public void setFinal();
       Code:
          0: aload_0
          1: iconst_1
          2: invokespecial #22                 // Method _isFinal_$eq:(Z)V
          5: return
   
   }
   
   So a call to setFinal directly calls _isFinal_$eq to set the variable. So we remove the one static setFinal call.
   
   With a public final flag and direct modification, the bytecode looks like this:
   
   public class PublicIsFinal {
     private boolean _isFinal;
   
     public boolean _isFinal();
       Code:
          0: aload_0
          1: getfield      #13                 // Field _isFinal:Z
          4: ireturn
   
     public void _isFinal_$eq(boolean);
       Code:
          0: aload_0
          1: iload_1
          2: putfield      #13                 // Field _isFinal:Z
          5: return
   }
   
   
   So _isFinal_$eq is called directly which modifies the field directly. So now we're down to a single function call.
   
   So we go from three functions calls with a trait, to one function call with public member. This feels like it shouldn't make that much of a difference, but it definitely did. It wasn't huge, and I'm not sure I would reccommend we do this everywhere because it does make readability/reusability more difficult, but in performance critical situations, avoiding traits and private vars seems to help a bit.
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org