groovy
  1. groovy
  2. GROOVY-5089

Unnecessary unboxing and casting in boolean handling in generated bytecode

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.8.3
    • Fix Version/s: 1.8.4, 2.0-beta-1
    • Component/s: None
    • Labels:
      None
    • Environment:
      Groovy 1.8.3 + Java 1.6.0_27 on Ubuntu Linux 10.04.3
    • Number of attachments :
      0

      Description

      This sample code:

      class GroovyBooleanTest {
      	public boolean someCall() {
      		return true;
      	}
      	
      	public void somecode() {
      		boolean val = someCall()
      		println val
      	}
      
      }
      

      produces very redundant bytecode (decompiled with jd-gui):

      import groovy.lang.GroovyObject;
      import groovy.lang.MetaClass;
      import org.codehaus.groovy.runtime.BytecodeInterface8;
      import org.codehaus.groovy.runtime.ScriptBytecodeAdapter;
      import org.codehaus.groovy.runtime.callsite.CallSite;
      import org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation;
      
      public class GroovyBooleanTest implements GroovyObject {
          public GroovyBooleanTest()
        {
          GroovyBooleanTest this;
          CallSite[] arrayOfCallSite = $getCallSiteArray();
          MetaClass localMetaClass = $getStaticMetaClass();
          this.metaClass = localMetaClass;
        }
      
          public boolean someCall() {
              CallSite[] arrayOfCallSite = $getCallSiteArray();
              return DefaultTypeTransformation.booleanUnbox(Boolean.TRUE);
              return DefaultTypeTransformation.booleanUnbox((Integer)DefaultTypeTransformation.box(0));
          }
      
          public void somecode() {
              CallSite[] arrayOfCallSite = $getCallSiteArray();
              boolean val = 0;
              Object localObject;
              boolean bool1;
              if ((!BytecodeInterface8.isOrigZ()) || (__$stMC) || (BytecodeInterface8.disabledStandardMetaClass())) {
                  localObject = arrayOfCallSite[0].callCurrent(this);
                  val = DefaultTypeTransformation.booleanUnbox((Boolean)ScriptBytecodeAdapter.castToType(localObject,
                          $get$$class$java$lang$Boolean()));
              }
              else {
                  bool1 = someCall();
                  val = DefaultTypeTransformation.booleanUnbox((Boolean)ScriptBytecodeAdapter.castToType(
                          (Boolean)DefaultTypeTransformation.box(bool1), $get$$class$java$lang$Boolean()));
              }
              arrayOfCallSite[1].callCurrent(this, (Boolean)DefaultTypeTransformation.box(val));
          }
      
          static {
              __$swapInit();
              Long localLong1 = (Long)DefaultTypeTransformation.box(0L);
              __timeStamp__239_neverHappen1319001147656 = DefaultTypeTransformation.longUnbox(localLong1);
              Long localLong2 = (Long)DefaultTypeTransformation.box(1319001147656L);
              __timeStamp = DefaultTypeTransformation.longUnbox(localLong2);
          }
      }
      

      This part of the bytecode is interesting:

      
                  bool1 = someCall();
                  val = DefaultTypeTransformation.booleanUnbox((Boolean)ScriptBytecodeAdapter.castToType(
                          (Boolean)DefaultTypeTransformation.box(bool1), $get$$class$java$lang$Boolean()));
      

      bool1 and val are both already booleans. It goes through many unnecessary layers before assigning val to bool1.

        Activity

        Hide
        Lari Hotari added a comment -

        What is also quite interesting is that the someCall method has 2 return statements in the bytecode

        (output produced by Eclipse Bytecode outline plugin: http://andrei.gmxhome.de/bytecode/)

          public someCall() : boolean
           L0
            INVOKESTATIC GroovyBooleanTest.$getCallSiteArray() : CallSite[]
            ASTORE 1
           L1
            LINENUMBER 4 L1
            GETSTATIC Boolean.TRUE : Boolean
            INVOKESTATIC DefaultTypeTransformation.booleanUnbox(Object) : boolean
            IRETURN
           L2
            LDC 0
            INVOKESTATIC DefaultTypeTransformation.box(int) : Object
            CHECKCAST Integer
            INVOKESTATIC DefaultTypeTransformation.booleanUnbox(Object) : boolean
            IRETURN
            LOCALVARIABLE this GroovyBooleanTest L0 L2 0
            MAXSTACK = 1
            MAXLOCALS = 2
        

        decompiled with JD-Gui:

            public boolean someCall() {
                CallSite[] arrayOfCallSite = $getCallSiteArray();
                return DefaultTypeTransformation.booleanUnbox(Boolean.TRUE);
                return DefaultTypeTransformation.booleanUnbox((Integer)DefaultTypeTransformation.box(0));
            }
        

        This must be a bug.

        Show
        Lari Hotari added a comment - What is also quite interesting is that the someCall method has 2 return statements in the bytecode (output produced by Eclipse Bytecode outline plugin: http://andrei.gmxhome.de/bytecode/ ) public someCall() : boolean L0 INVOKESTATIC GroovyBooleanTest.$getCallSiteArray() : CallSite[] ASTORE 1 L1 LINENUMBER 4 L1 GETSTATIC Boolean .TRUE : Boolean INVOKESTATIC DefaultTypeTransformation.booleanUnbox( Object ) : boolean IRETURN L2 LDC 0 INVOKESTATIC DefaultTypeTransformation.box( int ) : Object CHECKCAST Integer INVOKESTATIC DefaultTypeTransformation.booleanUnbox( Object ) : boolean IRETURN LOCALVARIABLE this GroovyBooleanTest L0 L2 0 MAXSTACK = 1 MAXLOCALS = 2 decompiled with JD-Gui: public boolean someCall() { CallSite[] arrayOfCallSite = $getCallSiteArray(); return DefaultTypeTransformation.booleanUnbox( Boolean .TRUE); return DefaultTypeTransformation.booleanUnbox(( Integer )DefaultTypeTransformation.box(0)); } This must be a bug.
        Hide
        Lari Hotari added a comment -

        What is this code generated in so many places:

        if ((!BytecodeInterface8.isOrigZ()) || (__$stMC) || (BytecodeInterface8.disabledStandardMetaClass())) {
        

        If that's really necessary, please "cache" the value of "(!BytecodeInterface8.isOrigZ()) || (__$stMC) || (BytecodeInterface8.disabledStandardMetaClass())" .

        Show
        Lari Hotari added a comment - What is this code generated in so many places: if ((!BytecodeInterface8.isOrigZ()) || (__$stMC) || (BytecodeInterface8.disabledStandardMetaClass())) { If that's really necessary, please "cache" the value of "(!BytecodeInterface8.isOrigZ()) || (__$stMC) || (BytecodeInterface8.disabledStandardMetaClass())" .
        Hide
        blackdrag blackdrag added a comment -

        this is not really a critical issue. As for the second return, it is dead code, it won't have any reasonable effect. The big IF you are wondering about, is the guard for the primitive optimization code, and it cannot be cached, unless you habe a suggestion. A better strategy would be to not to issue the optimization in this case. I have not yet added code that tests if the optimizations make sense in that case at all. Just returning a constant is maybe not a case in which we should do this kind of thing.

        Part of the optimizations I added is to gradually change values from being always boxed to be used native as they are. This is currently implemented only for int and double, but not for boolean. That is why you see the unboxing.

        Show
        blackdrag blackdrag added a comment - this is not really a critical issue. As for the second return, it is dead code, it won't have any reasonable effect. The big IF you are wondering about, is the guard for the primitive optimization code, and it cannot be cached, unless you habe a suggestion. A better strategy would be to not to issue the optimization in this case. I have not yet added code that tests if the optimizations make sense in that case at all. Just returning a constant is maybe not a case in which we should do this kind of thing. Part of the optimizations I added is to gradually change values from being always boxed to be used native as they are. This is currently implemented only for int and double, but not for boolean. That is why you see the unboxing.
        Hide
        Lari Hotari added a comment -

        Thanks for the answer. It would be cool to get the boolean optimizations too. Groovy doesn't look very clean when you go to the bytecode level without fixing this issue. Please schedule the change so that it gets fixed some time in the near future (1.8.4 / 1.8.5 ?).

        A more critical issue is GROOVY-5090 . I was actually hunting for that one (debugger showed "false" for a boolean value dispite the real value) when I noticed this unboxing/boxing redundancy.

        Show
        Lari Hotari added a comment - Thanks for the answer. It would be cool to get the boolean optimizations too. Groovy doesn't look very clean when you go to the bytecode level without fixing this issue. Please schedule the change so that it gets fixed some time in the near future (1.8.4 / 1.8.5 ?). A more critical issue is GROOVY-5090 . I was actually hunting for that one (debugger showed "false" for a boolean value dispite the real value) when I noticed this unboxing/boxing redundancy.
        Hide
        blackdrag blackdrag added a comment -

        fixed

        Show
        blackdrag blackdrag added a comment - fixed
        Hide
        Stéphane Maldini added a comment -

        Aleluia Thx thx thx thx

        Show
        Stéphane Maldini added a comment - Aleluia Thx thx thx thx
        Hide
        Nikolaj Jorgensen added a comment -

        Is it possible to add code to test if the optimizations make sense in order to remove unecessary if statements:

        if ((!BytecodeInterface8.isOrigZ()) || (__$stMC) || (BytecodeInterface8.disabledStandardMetaClass())) {
        

        I see this in situations where I do simple String assignments.
        Also it confuses Cobertura to think that the assignment is actually a branch condition.

        Show
        Nikolaj Jorgensen added a comment - Is it possible to add code to test if the optimizations make sense in order to remove unecessary if statements: if ((!BytecodeInterface8.isOrigZ()) || (__$stMC) || (BytecodeInterface8.disabledStandardMetaClass())) { I see this in situations where I do simple String assignments. Also it confuses Cobertura to think that the assignment is actually a branch condition.
        Hide
        blackdrag blackdrag added a comment -

        We have now tests that test that check the generated bytecode, so we can surely write tests. More important is to get a list of situations in which thee should be no optimization guard. String assignments normally don't trigger that.

        Maybe I should introduce some kind of optimization weight and a threshold, so that I optimize only if the weight is higher than the threshold. But even then I first need to know when it does not make sense.

        Show
        blackdrag blackdrag added a comment - We have now tests that test that check the generated bytecode, so we can surely write tests. More important is to get a list of situations in which thee should be no optimization guard. String assignments normally don't trigger that. Maybe I should introduce some kind of optimization weight and a threshold, so that I optimize only if the weight is higher than the threshold. But even then I first need to know when it does not make sense.

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Lari Hotari
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: