64788 - Make method invocation 10x faster with following code....

Status: VERIFIED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[David C. Navas]

Method.invoke() is an NMI call (JNI in 1.3 and up) and is not replaced when 
using the JIT.  That means that it's gawd-awful slow.  By replacing 
method.invoke() and getterSlot.getter.invoke() calls with on-the-fly generated 
stub code that calls the method in question directly, the invocation of methods 
can be sped up by an order of magnitude (or 20x when running under 1.3).

To hook the code in, keep a MyMethod instance in the FunctionObject and 
GetterSlot.  You may want to only keep a MyMethod instance for getters (setters 
can be slow, I would think, and may not warrant the extra space?).  You create 
the instances on-the-fly or during construction -- both seem to work -- by 
calling initializeMyMethod().  If you plan on creating instances on-the-fly, 
you should review the FunctionObject constructor.  It modifies the parameter 
types so that primitive types aren't primitive types anymore.  There may have 
originally been a reason for this code, but the parameter casting code checks 
both the ScriptRuntime classes and the primitive type classes when deciding how 
to perform the cast, so I'm not sure it needs to do that anymore.  <shrug>
I've removed that code without ill effect.

I have the MyMethod class as a static member of the FunctionObject class -- you 
may wish to change that.  You'll need to change some constants in the case.  
I've run the rhino regression suite and mozilla DOM tests, and they seem happy 
with this change....

Code for MyMethod (note, requires org.mozilla.classfile.* classes):
    public abstract static class MyMethod
    {
        static int _classNumber = 0;
        static Hashtable _myMethodHash = new Hashtable();
        static JavaAdapter.MyClassLoader _classLoader = new 
JavaAdapter.MyClassLoader();
            
        public static MyMethod initializeMyMethod(Method method, Class[] types)
        {
            MyMethod result = (MyMethod)_myMethodHash.get(method);
            
            if ( result != null )
                return result;
                
            int classNumber;
            
            synchronized(_myMethodHash)
            {
                classNumber = ++_classNumber;
            }
            
            String className = "js" + classNumber;
            ClassFileWriter cfw = new ClassFileWriter
(className, "org.mozilla.javascript.FunctionObject$MyMethod", "");
            cfw.setFlags((short)(ClassFileWriter.ACC_PUBLIC | 
ClassFileWriter.ACC_FINAL));
            
            // Add our instantiator!
            cfw.startMethod("<init>", "()V", ClassFileWriter.ACC_PUBLIC);
            cfw.add(ByteCode.ALOAD_0);
            cfw.add
(ByteCode.INVOKESPECIAL,"org.mozilla.javascript.FunctionObject$MyMethod",
                    "<init>", "()", "V");
            cfw.add(ByteCode.RETURN);
            cfw.stopMethod((short)1, null); // one argument -- this???

            // Add the invoke() method call
            cfw.startMethod("invoke", "(Ljava/lang/Object;[Ljava/lang/Object;)
Ljava/lang/Object;",
                            (short)(ClassFileWriter.ACC_PUBLIC | 
ClassFileWriter.ACC_FINAL));
            
            // If we return a primitive type, then do something special!
            String declaringClassName = method.getDeclaringClass().getName
().replace('.', '/');
            Class returnType = method.getReturnType();
            String invokeSpecial = null;
            String invokeSpecialType = null;
            boolean returnsVoid = false;
            boolean returnsBoolean = false;
            
            if ( returnType.isPrimitive() )
            {
                if ( returnType == Boolean.TYPE )
                {
                    returnsBoolean = true;
                    invokeSpecialType = "(Z)";
                }
                else if ( returnType == Void.TYPE )
                {
                    returnsVoid = true;
                    invokeSpecialType = "(V)";
                }
                else if ( returnType == Integer.TYPE )
                {
                    cfw.add(ByteCode.NEW, invokeSpecial = "java/lang/Integer");
                    cfw.add(ByteCode.DUP);
                    invokeSpecialType = "(I)";
                }
                else if ( returnType == Long.TYPE )
                {
                    cfw.add(ByteCode.NEW, invokeSpecial = "java/lang/Long");
                    cfw.add(ByteCode.DUP);
                    invokeSpecialType = "(J)";
                }
                else if ( returnType == Short.TYPE )
                {
                    cfw.add(ByteCode.NEW, invokeSpecial = "java/lang/Short");
                    cfw.add(ByteCode.DUP);
                    invokeSpecialType = "(S)";
                }
                else if ( returnType == Float.TYPE )
                {
                    cfw.add(ByteCode.NEW, invokeSpecial = "java/lang/Float");
                    cfw.add(ByteCode.DUP);
                    invokeSpecialType = "(F)";
                }
                else if ( returnType == Double.TYPE )
                {
                    cfw.add(ByteCode.NEW, invokeSpecial = "java/lang/Double");
                    cfw.add(ByteCode.DUP);
                    invokeSpecialType = "(D)";
                }
                else if ( returnType == Byte.TYPE )
                {
                    cfw.add(ByteCode.NEW, invokeSpecial = "java/lang/Byte");
                    cfw.add(ByteCode.DUP);
                    invokeSpecialType = "(B)";
                }
                else if ( returnType == Character.TYPE )
                {
                    cfw.add(ByteCode.NEW, invokeSpecial 
= "java/lang/Character");
                    cfw.add(ByteCode.DUP);
                    invokeSpecialType = "(C)";
                }
            }
            
            // handle setup of call to virtual function (if calling non-static)
            if ( !java.lang.reflect.Modifier.isStatic(method.getModifiers()) )
            {
                cfw.add(ByteCode.ALOAD_1);
                cfw.add(ByteCode.CHECKCAST, declaringClassName);
            }
            
            // Handle parameters!
            StringBuffer params = new StringBuffer( 2 + ((types!=null)?(20 * 
types.length):0) );
            
            params.append("(");
            if ( types != null )
            {
                for(int i = 0; i < types.length; i++)
                {
                    Class type = types[i];

                    cfw.add(ByteCode.ALOAD_2);
                    
                    if ( i <= 5) 
                    {
                        cfw.add((byte) (ByteCode.ICONST_0 + i));
                    }
                    else if (i <= Byte.MAX_VALUE)
                    {
                        cfw.add(ByteCode.BIPUSH, i);
                    }
                    else if (i <= Short.MAX_VALUE)
                    {
                        cfw.add(ByteCode.SIPUSH, i);
    	            }
    	            else 
    	            {
    	                cfw.addLoadConstant((int)i);
    	            }
                    
                    cfw.add(ByteCode.AALOAD);
                    
                    if ( type.isPrimitive() )
                    {
                        //Convert enclosed type back to primitive.
                        
                        if ( type == Boolean.TYPE )
                        {
                            cfw.add(ByteCode.CHECKCAST, "java/lang/Boolean");
                            cfw.add
(ByteCode.INVOKEVIRTUAL, "java/lang/Boolean", "booleanValue", 
                                    "()", "Z");
                            params.append("Z");
                        }
                        else if ( type == Integer.TYPE )
                        {
                            cfw.add(ByteCode.CHECKCAST, "java/lang/Number");
                            cfw.add
(ByteCode.INVOKEVIRTUAL, "java/lang/Number", "intValue", 
                                    "()", "I");
                            params.append("I");
                        }
                        else if ( type == Short.TYPE )
                        {
                            cfw.add(ByteCode.CHECKCAST, "java/lang/Number");
                            cfw.add
(ByteCode.INVOKEVIRTUAL, "java/lang/Number", "shortValue", 
                                    "()", "S");
                            params.append("S");
                        }
                        else if ( type == Character.TYPE )
                        {
                            cfw.add(ByteCode.CHECKCAST, "java/lang/Character");
                            cfw.add
(ByteCode.INVOKEVIRTUAL, "java/lang/Character", "charValue", 
                                    "()", "C");
                            params.append("C");
                        }
                        else if ( type == Double.TYPE )
                        {
                            cfw.add(ByteCode.CHECKCAST, "java/lang/Number");
                            cfw.add
(ByteCode.INVOKEVIRTUAL, "java/lang/Number", "doubleValue", 
                                    "()", "D");
                            params.append("D");
                        }
                        else if ( type == Float.TYPE )
                        {
                            cfw.add(ByteCode.CHECKCAST, "java/lang/Number");
                            cfw.add
(ByteCode.INVOKEVIRTUAL, "java/lang/Number", "floatValue", 
                                    "()", "F");
                            params.append("F");
                        }
                        else if ( type == Byte.TYPE )
                        {
                            cfw.add(ByteCode.CHECKCAST, "java/lang/Byte");
                            cfw.add
(ByteCode.INVOKEVIRTUAL, "java/lang/Byte", "byteValue", 
                                    "()", "B");
                            params.append("B");
                        }
                    }
                    else
                    {
                        String typeName = type.getName().replace('.', '/');
                        cfw.add(ByteCode.CHECKCAST, typeName);
                        
                        if ( !type.isArray() )
                        {
                            params.append('L');
                        }
                        params.append(typeName);
                        
                        if ( !type.isArray() )
                        {
                            params.append(';');
                        }
                    }
                }
            }
            params.append(")");
            
            // Call actual function!
            if ( !java.lang.reflect.Modifier.isStatic(method.getModifiers()) )
            {
                cfw.add(ByteCode.INVOKEVIRTUAL, declaringClassName, 
method.getName(), params.toString(),
                        (invokeSpecialType!=null?invokeSpecialType.substring
(1,2)
                                                :returnType.isArray()?
returnType.getName().replace('.','/')
                                                                    :"L".concat
(returnType.getName().replace('.', '/').concat(";"))));
            }
            else
            {
                cfw.add(ByteCode.INVOKESTATIC, declaringClassName, 
method.getName(), params.toString(),
                        (invokeSpecialType!=null?invokeSpecialType.substring
(1,2)
                                                :returnType.isArray()?
returnType.getName().replace('.','/')
                                                                    :"L".concat
(returnType.getName().replace('.', '/').concat(";"))));
            }
            
            // Handle return value;
            if ( returnsVoid )
            {
                cfw.add(ByteCode.ACONST_NULL);
                cfw.add(ByteCode.ARETURN);
            }
            else if ( returnsBoolean )
            {
                // HACK
                //check to see if true;
                // '7' is the number of bytes of the ifeq<branch> plus 
getstatic<TRUE> plus areturn instructions
                cfw.add(ByteCode.IFEQ, 7);
                cfw.add(ByteCode.GETSTATIC,
                        "java/lang/Boolean",
                                "TRUE",
                                "Ljava/lang/Boolean;");
                cfw.add(ByteCode.ARETURN);
                cfw.add(ByteCode.GETSTATIC,
                        "java/lang/Boolean",
                                "FALSE",
                                "Ljava/lang/Boolean;");
                cfw.add(ByteCode.ARETURN);
            }
            else if ( invokeSpecial != null )
            {
                cfw.add(ByteCode.INVOKESPECIAL,
                            invokeSpecial,
                            "<init>", invokeSpecialType, "V");
                cfw.add(ByteCode.ARETURN);
            }
            else
            {
                cfw.add(ByteCode.ARETURN);
            }
            cfw.stopMethod((short)3, null); // three arguments, including the 
this pointer???
            
            // Add class to our classloader.
            java.io.ByteArrayOutputStream bos = new 
java.io.ByteArrayOutputStream(550);
            
            try
            {
                cfw.write(bos);
            }
            catch (Throwable t)
            {
                t.printStackTrace();
            }
            
            try
            {
                byte [] bytes = bos.toByteArray();
                Class clazz = _classLoader.defineClass(className, bytes);
                result = (MyMethod)clazz.newInstance();
                
    //System.out.println("Generated method delegate for: " + method.getName() 
+ " on " + method.getDeclaringClass().getName() + " :: " + params.toString() 
+ " :: " + types);
                _myMethodHash.put(method, result);
            }
            catch (Throwable t)
            {
                t.printStackTrace();
            }

            return result;
        }
        
        public abstract Object invoke(Object that, Object [] args);
    }

Comment 1

[Norris Boyd]

Fantastic! 

Could you send me a diff for FunctionObject.java that shows how you call it? 
I've tried what I thought was obvious but it doesn't quite work.

Comment 2

[Norris Boyd]

Okay, I've got it running now. I'm seeing no difference on BenchPress.js, but
it doesn't call too many built-in functions. I made a simple test like 

function f() {
        for (var i=0; i < 10000; i++) {
                Math.abs(-1);
        }
}
var d = new Date();
f();
print((new Date()) - d);

I see a speedup of a little under 50%. I'll attach my diff so you can see if I
left out something. (Still a nice performance boost.)

Comment 3

[Norris Boyd]

Attachment: proposed patch with recommended changes

Comment 4

[David C. Navas]

Sorry to mislead -- the 10x performance is the performance boost seen from just 
the .invoke() viewpoint, not the boost seen through the entire javascript 
function call process (which includes looking up the function to call, possibly 
through scope or prototype chains, retrieving the Context from a hashtable, 
etc.).  Now, that would be too much to hope for!

The time spent in the test I was using decreased by about 30% overall, iirc.  
If it helps, the .js I'm using is:
    http://pages.ebay.com/included/js/cobrand/links.js
[yes, it would be easier to fix the javascript, but I don't have that 
luxury ;^/]


Looked at the proposed changes, four comments:

1) This diff has got to be wrong:
+                    type == Boolean.TYPE &&
+                    type == Byte.TYPE &&
+                    type == Short.TYPE &&
+                    type == Integer.TYPE &&
+                    type == Float.TYPE && 
+                    type == Double.TYPE)

There's no way for that to ever be true!
The only thing I changed there was to remove these lines
<                     types[i] = ScriptRuntime.BooleanClass;
178d561
<                     types[i] = ScriptRuntime.ByteClass;
181d563
<                     types[i] = ScriptRuntime.ShortClass;
184d565
<                     types[i] = ScriptRuntime.IntegerClass;
187d567
<                     types[i] = ScriptRuntime.FloatClass;
190d569
<                     types[i] = ScriptRuntime.DoubleClass;

2) The constructor for FunctionObject sometimes nulls out the types
   array (if there is no casting required).  I don't think your change took
   that into account???  I grab the paramTypes of the method again if types
   is null.  No biggie....

3) There are two methods in FunctionObject, one in call() and one in
   callVarargs(), that can use MyMethod

4) There is one place in ScriptableObject (getter invoke) you might want to
   consider as well.  Most of the code I was dealing with was actually calling
   getters (getDocument(), getLinks(), getHref(), etc., as you might be able
   to tell from the above .js file)

I'd send a FunctionObject diff to you, but I've made so many tweaks to the
file at this point that it might call functions or reference classes that don't 
exist in your version (getParameterTypes() and NativeDelegate I think fall into 
that category).  But, for what it's worth, aside from the MyMethod stuff I 
already sent, and the import line:
***************
*** 171,194 ****
                  {
                      hasConversions = true;
                  } else if (type == Boolean.TYPE) {
                      hasConversions = true;
-                     types[i] = ScriptRuntime.BooleanClass;
                  } else if (type == Byte.TYPE) {
                      hasConversions = true;
-                     types[i] = ScriptRuntime.ByteClass;
                  } else if (type == Short.TYPE) {
                      hasConversions = true;
-                     types[i] = ScriptRuntime.ShortClass;
                  } else if (type == Integer.TYPE) {
                      hasConversions = true;
-                     types[i] = ScriptRuntime.IntegerClass;
                  } else if (type == Float.TYPE) {
                      hasConversions = true;
-                     types[i] = ScriptRuntime.FloatClass;
                  } else if (type == Double.TYPE) {
                      hasConversions = true;
-                     types[i] = ScriptRuntime.DoubleClass;
                  } else if ( type.isInterface() )
                  {
                      //Pretend like none is needed.
                      //hasConversions = true;
--- 556,573 ----
***************
*** 592,601 ****
              if ( thisObj instanceof NativeDelegate )
              {
                  thisObj = thisObj.getPrototype();
              }
!             Object result = (method != null)
!                             ? method.invoke(thisObj, invokeArgs)
                              : ctor.newInstance(invokeArgs);
              return hasVoidReturn ? Undefined.instance : result;
          }
          catch (InvocationTargetException e) {
--- 971,986 ----
              if ( thisObj instanceof NativeDelegate )
              {
                  thisObj = thisObj.getPrototype();
              }
!
!             if ( myMethod == null && method != null)
!             {
!                 // Initialize the myMethod correctly!
!                 myMethod = MyMethod.initializeMyMethod(method, (types==null?ge
tParameterTypes(method):types));
!             }
!
!             Object result = (myMethod!=null)? myMethod.invoke(thisObj, invokeA
rgs)
                              : ctor.newInstance(invokeArgs);
              return hasVoidReturn ? Undefined.instance : result;
          }
          catch (InvocationTargetException e) {
***************
*** 661,677 ****
          throws JavaScriptException
      {
          try {
              if (parmsLength == VARARGS_METHOD) {
!                 Object[] invokeArgs = { cx, thisObj, args, this };
!                 Object result = method.invoke(null, invokeArgs);
                  return hasVoidReturn ? Undefined.instance : result;
              } else {
                  Boolean b = inNewExpr ? Boolean.TRUE : Boolean.FALSE;
!                 Object[] invokeArgs = { cx, args, this, b };
!                 return (method == null)
!                        ? ctor.newInstance(invokeArgs)
!                        : method.invoke(null, invokeArgs);
              }
          }
          catch (InvocationTargetException e) {
              Throwable target = e.getTargetException();
--- 1046,1073 ----
          throws JavaScriptException
      {
          try {
              if (parmsLength == VARARGS_METHOD) {
!                 Object[] invokeArgs = cx.getStaticFunctionArgs( thisObj, args,
 this );
!                 if ( myMethod == null)
!                 {
!                     // Initialize the myMethod correctly!
!                     myMethod = MyMethod.initializeMyMethod(method, (types==nul
l?getParameterTypes(method):types));
!                 }
!                 Object result = myMethod.invoke(null, invokeArgs);
                  return hasVoidReturn ? Undefined.instance : result;
              } else {
                  Boolean b = inNewExpr ? Boolean.TRUE : Boolean.FALSE;
!                 Object[] invokeArgs = cx.getStaticFunctionArgs( args, this, b
);
!
!                 if ( myMethod == null && method != null)
!                 {
!                     // Initialize the myMethod correctly!
!                     myMethod = MyMethod.initializeMyMethod(method, (types==nul
l?getParameterTypes(method):types));
!                 }
!
!                 return (myMethod!=null)?myMethod.invoke(null, invokeArgs)
!                                        :ctor.newInstance(invokeArgs);
              }
          }
          catch (InvocationTargetException e) {
              Throwable target = e.getTargetException();
***************
*** 704,711 ****
--- 1100,1108 ----
      private static boolean sawSecurityException;

      static Method[] methodsCache;

+     MyMethod myMethod;
      Method method;
      Constructor ctor;
      private Class[] types;
      private short parmsLength;

Comment 5

[Norris Boyd]

Fixed:

Checking in Context.java;
/cvsroot/mozilla/js/rhino/org/mozilla/javascript/Context.java,v  <--  Context.ja
va
new revision: 1.47; previous revision: 1.46
done
Checking in FunctionObject.java;
/cvsroot/mozilla/js/rhino/org/mozilla/javascript/FunctionObject.java,v  <--  Fun
ctionObject.java
new revision: 1.20; previous revision: 1.19
done
Checking in JavaAdapter.java;
/cvsroot/mozilla/js/rhino/org/mozilla/javascript/JavaAdapter.java,v  <--  JavaAd
apter.java
new revision: 1.36; previous revision: 1.35
done

Comment 6

[Phil Schwartau]

Marking Verified -