groovy
  1. groovy
  2. GROOVY-5059

Improve DefaultTypeTransformation.booleanUnbox performance

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.9-beta-4, 1.8.4, 1.7.11
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      1

      Description

      In profiling a Grails application, org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation.booleanUnbox(Object) is showing up as one of the top "blockers" (groovy.lang.ExpandoMetaClass.isInitialized() is actually causing the blocking since EMC gets always used in Grails).

      The booleanUnbox/castToBoolean method should be optimized for the case when the Object is already a boolean. Currently it goes threw a lot of unnecessary layers also for boolean values.

      current implementation:

          public static boolean castToBoolean(Object object) {
              // null is always false
              if (object == null) {
                  return false;
              }
      
              // if the object is not null, try to call an asBoolean() method on the object
              return (Boolean)InvokerHelper.invokeMethod(object, "asBoolean", InvokerHelper.EMPTY_ARGS);
          }
      

      improved implementation:

          public static boolean castToBoolean(Object object) {
              // null is always false
              if (object == null) {
                  return false;
              }
              if (object.getClass() == Boolean.class) {   // equality check is enough and faster than instanceof check, no need to check superclasses since Boolean is final
                  return ((Boolean)object).booleanValue();
              }
              // if the object is not null, try to call an asBoolean() method on the object
              return (Boolean)InvokerHelper.invokeMethod(object, "asBoolean", InvokerHelper.EMPTY_ARGS);
          }
      

        Activity

        Hide
        Lari Hotari added a comment -

        Screenshot of a YJP profiling session. It shows that 96% of thread blocking is caused by EMC.isInitialized() (GROOVY-3557) and of that 91% of is caused by DTT.booleanUnbox(Object).

        Show
        Lari Hotari added a comment - Screenshot of a YJP profiling session. It shows that 96% of thread blocking is caused by EMC.isInitialized() ( GROOVY-3557 ) and of that 91% of is caused by DTT.booleanUnbox(Object).
        Hide
        blackdrag blackdrag added a comment -

        the point though is mostly about... if a user provides an Boolean#asBoolean method, or wants to hook in the invocation of that method, is it legal for us to ignore that will and bypass it completely?

        Show
        blackdrag blackdrag added a comment - the point though is mostly about... if a user provides an Boolean#asBoolean method, or wants to hook in the invocation of that method, is it legal for us to ignore that will and bypass it completely?
        Hide
        Lari Hotari added a comment -

        I added btrace profiling to get statistics of booleanUnbox.

        Here is the code for the btrace "script":

        import com.sun.btrace.annotations.*;
        import static com.sun.btrace.BTraceUtils.*;
        import com.sun.btrace.AnyType;
        import java.util.Map;
        import java.util.concurrent.atomic.AtomicInteger;
        
        @BTrace public class BooleanUnboxHistogram {
           private static Map<String, AtomicInteger> histo = Collections.newHashMap();
           private static AtomicInteger total = Atomic.newAtomicInteger(1);
        
            @OnMethod(  
                clazz="org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation",  
                method="booleanUnbox"
            )  
            public static void onBooleanUnbox(AnyType[] arg) {
        	if (arg[0] != null) {
        	    String cn = Reflective.name(classOf(arg[0]));
        	    AtomicInteger ai = Collections.get(histo, cn);
        	    if (ai == null) {
        		ai = Atomic.newAtomicInteger(1);
        		Collections.put(histo, cn, ai);
        	    } else {
        		Atomic.incrementAndGet(ai);
        	    }
        	    Atomic.incrementAndGet(total);
        	}
            }
        
            @OnTimer(5000) 
            public static void print() {
                if (Collections.size(histo) != 0) {
                    printNumberMap("\n\nArgument Histogram for booleanUnbox", histo);
                    printNumber("Total", Atomic.get(total));
                }
            }
        }
        

        For a single real-world Grails app page refresh booleanUnbox was called 3092 times and 1995 of those were type of java.lang.Boolean.
        That's 65%.

        Show
        Lari Hotari added a comment - I added btrace profiling to get statistics of booleanUnbox. Here is the code for the btrace "script": import com.sun.btrace.annotations.*; import static com.sun.btrace.BTraceUtils.*; import com.sun.btrace.AnyType; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; @BTrace public class BooleanUnboxHistogram { private static Map< String , AtomicInteger> histo = Collections.newHashMap(); private static AtomicInteger total = Atomic.newAtomicInteger(1); @OnMethod( clazz= "org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation" , method= "booleanUnbox" ) public static void onBooleanUnbox(AnyType[] arg) { if (arg[0] != null ) { String cn = Reflective.name(classOf(arg[0])); AtomicInteger ai = Collections.get(histo, cn); if (ai == null ) { ai = Atomic.newAtomicInteger(1); Collections.put(histo, cn, ai); } else { Atomic.incrementAndGet(ai); } Atomic.incrementAndGet(total); } } @OnTimer(5000) public static void print() { if (Collections.size(histo) != 0) { printNumberMap( "\n\nArgument Histogram for booleanUnbox" , histo); printNumber( "Total" , Atomic.get(total)); } } } For a single real-world Grails app page refresh booleanUnbox was called 3092 times and 1995 of those were type of java.lang.Boolean. That's 65%.
        Hide
        Lari Hotari added a comment - - edited

        I re-indexed the Grails Searchable index and looked at the statistics after that:
        java.lang.Boolean instances: 118384 / 159677 = 74%.
        java.lang.Number instances: 10356 / 159677 = 6,5%
        java.lang.Collection instances: 10212 / 159677 = 6,4%
        java.lang.String instances: 9271 / 159677 = 5,8%
        Adding asBoolean bypass for all of these would give 93% coverage. (In the testcase I did the test for...)

        Show
        Lari Hotari added a comment - - edited I re-indexed the Grails Searchable index and looked at the statistics after that: java.lang.Boolean instances: 118384 / 159677 = 74%. java.lang.Number instances: 10356 / 159677 = 6,5% java.lang.Collection instances: 10212 / 159677 = 6,4% java.lang.String instances: 9271 / 159677 = 5,8% Adding asBoolean bypass for all of these would give 93% coverage. (In the testcase I did the test for...)
        Hide
        Lari Hotari added a comment -

        The old while(true) issue is related: GROOVY-3123 .

        Show
        Lari Hotari added a comment - The old while(true) issue is related: GROOVY-3123 .
        Hide
        blackdrag blackdrag added a comment -

        I was a bit unsure about if that change is legal, but I think it is now, so I committed the change. Thanks for finding this

        Show
        blackdrag blackdrag added a comment - I was a bit unsure about if that change is legal, but I think it is now, so I committed the change. Thanks for finding this

          People

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

            Dates

            • Created:
              Updated:
              Resolved: