groovy

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

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
blackdrag blackdrag added a comment -

fixed

Show
blackdrag blackdrag added a comment - fixed
blackdrag blackdrag made changes -
Field Original Value New Value
Status Open [ 1 ] Closed [ 6 ]
Assignee Jochen Theodorou [ blackdrag ]
Resolution Fixed [ 1 ]
Hide
Permalink
Stéphane Maldini added a comment -

Aleluia Thx thx thx thx

Show
Stéphane Maldini added a comment - Aleluia Thx thx thx thx
blackdrag blackdrag made changes -
Resolution Fixed [ 1 ]
Status Closed [ 6 ] Reopened [ 4 ]
blackdrag blackdrag made changes -
Status Reopened [ 4 ] Closed [ 6 ]
Fix Version/s 1.9-beta-x [ 17013 ]
Resolution Fixed [ 1 ]
Hide
Permalink
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
Permalink
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.
Paul King made changes -
Resolution Fixed [ 1 ]
Status Closed [ 6 ] Reopened [ 4 ]
Paul King made changes -
Fix Version/s 2.0-beta-x [ 17013 ]
Fix Version/s 1.8.4 [ 17852 ]
Paul King made changes -
Fix Version/s 2.0-beta-1 [ 17925 ]
Paul King made changes -
Status Reopened [ 4 ] Resolved [ 5 ]
Resolution Fixed [ 1 ]
blackdrag blackdrag made changes -
Resolution Fixed [ 1 ]
Status Resolved [ 5 ] Reopened [ 4 ]
blackdrag blackdrag made changes -
Status Reopened [ 4 ] Closed [ 6 ]
Fix Version/s 1.8.4 [ 17852 ]
Resolution Fixed [ 1 ]

People

  • Assignee:
    blackdrag blackdrag
    Reporter:
    Lari Hotari
Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: