From 0083d37b0e1c9e542f671cbca2e9db6819ecccba Mon Sep 17 00:00:00 2001 From: Andy McFadden Date: Fri, 21 Aug 2009 14:44:04 -0700 Subject: [PATCH] Use local references for native method args. This changes the JNI method call mechanism to register all reference arguments as local refs, as well as the "this" argument (for virtual calls) and the class object (for static calls). Before now we skipped this part, because we're handing raw object pointers around, and we know that all of the arguments can be found by the GC on the interpreted stack. In fact, there's no need to add the arguments for GC-correctness; rather, we need to do this to rewrite the pointers we hand to native code. This change impacts JNI method call performance, especially for functions with a large number of reference arguments. To improve things a little, there are now four "call bridge" functions: (1) general handler (2) synchronized method handler (grabs lock, calls #1) (3) virtual method, no reference args (4) static method, no reference args While booting the system, the virtual/static no-ref handlers are used for about 70% of calls. Since these don't have to scan the method signature to look for reference arguments, they're a bit faster. At this point we're still passing raw pointers around, so nothing should really change. --- vm/CheckJni.c | 53 +++++++--- vm/Jni.c | 282 ++++++++++++++++++++++++++++++++++++++++++++---------- vm/JniInternal.h | 20 ++-- vm/Native.c | 2 +- vm/Thread.h | 4 +- vm/interp/Stack.c | 97 +++++++++---------- 6 files changed, 335 insertions(+), 123 deletions(-) diff --git a/vm/CheckJni.c b/vm/CheckJni.c index 390f4c8fb..31c35c323 100644 --- a/vm/CheckJni.c +++ b/vm/CheckJni.c @@ -112,31 +112,60 @@ static void checkCallCommon(const u4* args, JValue* pResult, } /* + * Determine if we need to check the return type coming out of the call. + * + * (We don't do this at the top of checkCallCommon() because this is on + * the critical path for native method calls.) + */ +static inline bool callNeedsCheck(const u4* args, JValue* pResult, + const Method* method, Thread* self) +{ + return (method->shorty[0] == 'L' && !dvmCheckException(self) && + pResult->l != NULL); +} + +/* * Check a call into native code. */ -void dvmCheckCallJNIMethod(const u4* args, JValue* pResult, +void dvmCheckCallJNIMethod_general(const u4* args, JValue* pResult, const Method* method, Thread* self) { - dvmCallJNIMethod(args, pResult, method, self); - if (method->shorty[0] == 'L' && !dvmCheckException(self) && - pResult->l != NULL) - { + dvmCallJNIMethod_general(args, pResult, method, self); + if (callNeedsCheck(args, pResult, method, self)) checkCallCommon(args, pResult, method, self); - } } /* * Check a synchronized call into native code. */ -void dvmCheckCallSynchronizedJNIMethod(const u4* args, JValue* pResult, +void dvmCheckCallJNIMethod_synchronized(const u4* args, JValue* pResult, const Method* method, Thread* self) { - dvmCallSynchronizedJNIMethod(args, pResult, method, self); - if (method->shorty[0] == 'L' && !dvmCheckException(self) && - pResult->l != NULL) - { + dvmCallJNIMethod_synchronized(args, pResult, method, self); + if (callNeedsCheck(args, pResult, method, self)) + checkCallCommon(args, pResult, method, self); +} + +/* + * Check a virtual call with no reference arguments (other than "this"). + */ +void dvmCheckCallJNIMethod_virtualNoRef(const u4* args, JValue* pResult, + const Method* method, Thread* self) +{ + dvmCallJNIMethod_virtualNoRef(args, pResult, method, self); + if (callNeedsCheck(args, pResult, method, self)) + checkCallCommon(args, pResult, method, self); +} + +/* + * Check a static call with no reference arguments (other than "clazz"). + */ +void dvmCheckCallJNIMethod_staticNoRef(const u4* args, JValue* pResult, + const Method* method, Thread* self) +{ + dvmCallJNIMethod_staticNoRef(args, pResult, method, self); + if (callNeedsCheck(args, pResult, method, self)) checkCallCommon(args, pResult, method, self); - } } diff --git a/vm/Jni.c b/vm/Jni.c index 4df32b1aa..abd520a9b 100644 --- a/vm/Jni.c +++ b/vm/Jni.c @@ -48,19 +48,19 @@ checked at runtime, such as array bounds checks, we do the tests here. General notes on local/global reference tracking -JNI provides explicit control over natively-held references that the VM GC +JNI provides explicit control over natively-held references that the GC needs to know about. These can be local, in which case they're released -when the native method returns, or global, which are held until explicitly -released. - -The references can be created and deleted with JNI NewLocalRef / -NewGlobalRef calls, but this is unusual except perhaps for holding on -to a Class reference. Most often they are created transparently by the -JNI functions. For example, the paired Get/Release calls guarantee that -objects survive until explicitly released, so a simple way to implement -this is to create a global reference on "Get" and delete it on "Release". -The AllocObject/NewObject functions must create local references, because -nothing else in the GC root set has a reference to the new objects. +when the native method returns into the VM, or global, which are held +until explicitly released. + +The references can be created with explicit JNI NewLocalRef / NewGlobalRef +calls. The former is very unusual, the latter is reasonably common +(e.g. for caching references to class objects). + +Local references are most often created as a side-effect of JNI functions. +For example, the AllocObject/NewObject functions must create local +references to the objects returned, because nothing else in the GC root +set has a reference to the new objects. The most common mode of operation is for a method to create zero or more local references and return. Explicit "local delete" operations @@ -79,9 +79,7 @@ to move any). The spec says, "Local references are only valid in the thread in which they are created. The native code must not pass local references from -one thread to another." It should also be noted that, while some calls -will *create* global references as a side-effect, only the NewGlobalRef -and NewWeakGlobalRef calls actually *return* global references. +one thread to another." Global reference tracking @@ -220,6 +218,7 @@ Compared to #1, approach #2: /* fwd */ static const struct JNINativeInterface gNativeInterface; +static jobject addLocalReference(JNIEnv* env, Object* obj); static jobject addGlobalReference(Object* obj); @@ -241,37 +240,112 @@ static void checkStackSum(Thread* self); */ /* - * Bridge to calling a JNI function. This ideally gets some help from - * assembly language code in dvmPlatformInvoke, because the arguments - * must be pushed into the native stack as if we were calling a - * function. + * The functions here form a bridge between interpreted code and JNI native + * functions. The basic task is to convert an array of primitives and + * references into C-style function arguments. This is architecture-specific + * and usually requires help from assembly code. + * + * The bridge takes four arguments: the array of parameters, a place to + * store the function result (if any), the method to call, and a pointer + * to the current thread. + * + * These functions aren't called directly from elsewhere in the VM. + * A pointer in the Method struct points to one of these, and when a native + * method is invoked the interpreter jumps to it. + * + * (The "internal native" methods are invoked the same way, but instead + * of calling through a bridge, the target method is called directly.) * - * The number of values in "args" must match method->insSize. + * The "args" array should not be modified, but we do so anyway for + * performance reasons. We know that it points to the "outs" area on + * the current method's interpreted stack. This area is ignored by the + * precise GC, because there is no register map for a native method (for + * an interpreted method the args would be listed in the argument set). + * We know all of the values exist elsewhere on the interpreted stack, + * because the method call setup copies them right before making the call, + * so we don't have to worry about concealing stuff from the GC. * - * This is generally just set up by the resolver and then called through. - * We don't call here explicitly. This takes the same arguments as all - * of the "internal native" methods. + * If we don't want to modify "args", we either have to create a local + * copy and modify it before calling dvmPlatformInvoke, or we have to do + * the local reference replacement within dvmPlatformInvoke. The latter + * has some performance advantages, though if we can inline the local + * reference adds we may win when there's a lot of reference args (unless + * we want to code up some local ref table manipulation in assembly. */ -void dvmCallJNIMethod(const u4* args, JValue* pResult, const Method* method, - Thread* self) + +/* + * General form, handles all cases. + */ +void dvmCallJNIMethod_general(const u4* args, JValue* pResult, + const Method* method, Thread* self) { int oldStatus; + u4* modArgs = (u4*) args; assert(method->insns != NULL); - //int i; - //LOGI("JNI calling %p (%s.%s %s):\n", method->insns, - // method->clazz->descriptor, method->name, method->signature); - //for (i = 0; i < method->insSize; i++) - // LOGI(" %d: 0x%08x\n", i, args[i]); + //LOGI("JNI calling %p (%s.%s:%s):\n", method->insns, + // method->clazz->descriptor, method->name, method->shorty); + + /* + * Walk the argument list, creating local references for appropriate + * arguments. + */ + JNIEnv* env = self->jniEnv; + jclass staticMethodClass; + int idx = 0; + if (dvmIsStaticMethod(method)) { + /* add the class object we pass in */ + staticMethodClass = addLocalReference(env, (Object*) method->clazz); + if (staticMethodClass == NULL) { + assert(dvmCheckException(self)); + return; + } + } else { + /* add "this" */ + staticMethodClass = NULL; + jobject thisObj = addLocalReference(env, (Object*) modArgs[0]); + if (thisObj == NULL) { + assert(dvmCheckException(self)); + return; + } + modArgs[idx] = (u4) thisObj; + idx = 1; + } + + const char* shorty = &method->shorty[1]; /* skip return type */ + while (*shorty != '\0') { + switch (*shorty++) { + case 'L': + //LOGI(" local %d: 0x%08x\n", idx, modArgs[idx]); + if (modArgs[idx] != 0) { + //if (!dvmIsValidObject((Object*) modArgs[idx])) + // dvmAbort(); + jobject argObj = addLocalReference(env, (Object*) modArgs[idx]); + if (argObj == NULL) { + assert(dvmCheckException(self)); + return; + } + modArgs[idx] = (u4) argObj; + } + break; + case 'D': + case 'J': + idx++; + break; + default: + /* Z B C S I -- do nothing */ + break; + } + + idx++; + } oldStatus = dvmChangeStatus(self, THREAD_NATIVE); COMPUTE_STACK_SUM(self); - // TODO: should we be converting 'this' to a local ref? - dvmPlatformInvoke(self->jniEnv, - dvmIsStaticMethod(method) ? method->clazz : NULL, - method->jniArgInfo, method->insSize, args, method->shorty, + dvmPlatformInvoke(self->jniEnv, staticMethodClass, + method->jniArgInfo, method->insSize, modArgs, method->shorty, (void*)method->insns, pResult); CHECK_STACK_SUM(self); @@ -279,11 +353,11 @@ void dvmCallJNIMethod(const u4* args, JValue* pResult, const Method* method, } /* - * Alternate call bridge for the unusual case of a synchronized native method. + * Handler for the unusual case of a synchronized native method. * - * Lock the object, then call through the usual function. + * Lock the object, then call through the general function. */ -void dvmCallSynchronizedJNIMethod(const u4* args, JValue* pResult, +void dvmCallJNIMethod_synchronized(const u4* args, JValue* pResult, const Method* method, Thread* self) { Object* lockObj; @@ -300,11 +374,68 @@ void dvmCallSynchronizedJNIMethod(const u4* args, JValue* pResult, lockObj, lockObj->clazz->descriptor); dvmLockObject(self, lockObj); - dvmCallJNIMethod(args, pResult, method, self); + dvmCallJNIMethod_general(args, pResult, method, self); dvmUnlockObject(self, lockObj); } /* + * Virtual method call, no reference arguments. + * + * We need to local-ref the "this" argument, found in args[0]. + */ +void dvmCallJNIMethod_virtualNoRef(const u4* args, JValue* pResult, + const Method* method, Thread* self) +{ + u4* modArgs = (u4*) args; + int oldStatus; + + jobject thisObj = addLocalReference(self->jniEnv, (Object*) args[0]); + if (thisObj == NULL) { + assert(dvmCheckException(self)); + return; + } + modArgs[0] = (u4) thisObj; + + oldStatus = dvmChangeStatus(self, THREAD_NATIVE); + + COMPUTE_STACK_SUM(self); + dvmPlatformInvoke(self->jniEnv, NULL, + method->jniArgInfo, method->insSize, modArgs, method->shorty, + (void*)method->insns, pResult); + CHECK_STACK_SUM(self); + + dvmChangeStatus(self, oldStatus); +} + +/* + * Static method call, no reference arguments. + * + * We need to local-ref the class reference. + */ +void dvmCallJNIMethod_staticNoRef(const u4* args, JValue* pResult, + const Method* method, Thread* self) +{ + jclass staticMethodClass; + int oldStatus; + + staticMethodClass = addLocalReference(self->jniEnv, (Object*)method->clazz); + if (staticMethodClass == NULL) { + assert(dvmCheckException(self)); + return; + } + + oldStatus = dvmChangeStatus(self, THREAD_NATIVE); + + COMPUTE_STACK_SUM(self); + dvmPlatformInvoke(self->jniEnv, staticMethodClass, + method->jniArgInfo, method->insSize, args, method->shorty, + (void*)method->insns, pResult); + CHECK_STACK_SUM(self); + + dvmChangeStatus(self, oldStatus); +} + +/* * Extract the return type enum from the "jniArgInfo" field. */ DalvikJniReturnType dvmGetArgInfoReturnType(int jniArgInfo) @@ -615,9 +746,10 @@ static jobject addLocalReference(JNIEnv* env, Object* obj) dvmDumpThread(dvmThreadSelf(), false); dvmAbort(); // spec says call FatalError; this is equivalent } else { - LOGVV("LREF add %p (%s.%s)\n", obj, + LOGVV("LREF add %p (%s.%s) (ent=%d)\n", obj, dvmGetCurrentJNIMethod()->clazz->descriptor, - dvmGetCurrentJNIMethod()->name); + dvmGetCurrentJNIMethod()->name, + (int) dvmReferenceTableEntries(pRefTable)); } return obj; @@ -901,6 +1033,7 @@ void dvmGcMarkJniGlobalRefs() } +#if 0 /* * Determine if "obj" appears in the argument list for the native method. * @@ -971,16 +1104,17 @@ static bool findInArgList(Thread* self, Object* obj) } return false; } +#endif /* * Verify that a reference passed in from native code is one that the * code is allowed to have. * * It's okay for native code to pass us a reference that: - * - was just passed in as an argument when invoked by native code - * - was returned to it from JNI (and is now in the JNI local refs table) + * - was passed in as an argument when invoked by native code (and hence + * is in the JNI local refs table) + * - was returned to it from JNI (and is now in the local refs table) * - is present in the JNI global refs table - * The first one is a little awkward. The latter two are just table lookups. * * Used by -Xcheck:jni and GetObjectRefType. * @@ -995,11 +1129,13 @@ jobjectRefType dvmGetJNIRefType(JNIEnv* env, jobject jobj) //Object** top; Object** ptr; +#if 0 /* check args */ if (findInArgList(self, jobj)) { //LOGI("--- REF found %p on stack\n", jobj); return JNILocalRefType; } +#endif /* check locals */ //top = SAVEAREA_FROM_FP(self->curFrame)->xtra.localRefTop; @@ -1081,16 +1217,60 @@ static bool dvmIsCheckJNIEnabled(void) */ void dvmUseJNIBridge(Method* method, void* func) { + enum { + kJNIGeneral = 0, + kJNISync = 1, + kJNIVirtualNoRef = 2, + kJNIStaticNoRef = 3, + } kind; + static const DalvikBridgeFunc stdFunc[] = { + dvmCallJNIMethod_general, + dvmCallJNIMethod_synchronized, + dvmCallJNIMethod_virtualNoRef, + dvmCallJNIMethod_staticNoRef + }; + static const DalvikBridgeFunc checkFunc[] = { + dvmCheckCallJNIMethod_general, + dvmCheckCallJNIMethod_synchronized, + dvmCheckCallJNIMethod_virtualNoRef, + dvmCheckCallJNIMethod_staticNoRef + }; + + bool hasRefArg = false; + + if (dvmIsSynchronizedMethod(method)) { + /* use version with synchronization; calls into general handler */ + kind = kJNISync; + } else { + /* + * Do a quick scan through the "shorty" signature to see if the method + * takes any reference arguments. + */ + const char* cp = method->shorty; + while (*++cp != '\0') { /* pre-incr to skip return type */ + if (*cp == 'L') { + /* 'L' used for both object and array references */ + hasRefArg = true; + break; + } + } + + if (hasRefArg) { + /* use general handler to slurp up reference args */ + kind = kJNIGeneral; + } else { + /* virtual methods have a ref in args[0] (not in signature) */ + if (dvmIsStaticMethod(method)) + kind = kJNIStaticNoRef; + else + kind = kJNIVirtualNoRef; + } + } + if (dvmIsCheckJNIEnabled()) { - if (dvmIsSynchronizedMethod(method)) - dvmSetNativeFunc(method, dvmCheckCallSynchronizedJNIMethod, func); - else - dvmSetNativeFunc(method, dvmCheckCallJNIMethod, func); + dvmSetNativeFunc(method, checkFunc[kind], func); } else { - if (dvmIsSynchronizedMethod(method)) - dvmSetNativeFunc(method, dvmCallSynchronizedJNIMethod, func); - else - dvmSetNativeFunc(method, dvmCallJNIMethod, func); + dvmSetNativeFunc(method, stdFunc[kind], func); } } diff --git a/vm/JniInternal.h b/vm/JniInternal.h index cbe9d8ad2..f39583e8f 100644 --- a/vm/JniInternal.h +++ b/vm/JniInternal.h @@ -119,17 +119,25 @@ INLINE void dvmSetJniEnvThreadId(JNIEnv* pEnv, Thread* self) } /* - * JNI call bridges. Not usually called directly. + * JNI call bridges. Not called directly. * * The "Check" versions are used when CheckJNI is enabled. */ -void dvmCallJNIMethod(const u4* args, JValue* pResult, const Method* method, - Thread* self); -void dvmCallSynchronizedJNIMethod(const u4* args, JValue* pResult, +void dvmCallJNIMethod_general(const u4* args, JValue* pResult, const Method* method, Thread* self); -void dvmCheckCallJNIMethod(const u4* args, JValue* pResult, +void dvmCallJNIMethod_synchronized(const u4* args, JValue* pResult, const Method* method, Thread* self); -void dvmCheckCallSynchronizedJNIMethod(const u4* args, JValue* pResult, +void dvmCallJNIMethod_virtualNoRef(const u4* args, JValue* pResult, + const Method* method, Thread* self); +void dvmCallJNIMethod_staticNoRef(const u4* args, JValue* pResult, + const Method* method, Thread* self); +void dvmCheckCallJNIMethod_general(const u4* args, JValue* pResult, + const Method* method, Thread* self); +void dvmCheckCallJNIMethod_synchronized(const u4* args, JValue* pResult, + const Method* method, Thread* self); +void dvmCheckCallJNIMethod_virtualNoRef(const u4* args, JValue* pResult, + const Method* method, Thread* self); +void dvmCheckCallJNIMethod_staticNoRef(const u4* args, JValue* pResult, const Method* method, Thread* self); /* diff --git a/vm/Native.c b/vm/Native.c index 1892618d6..31832c250 100644 --- a/vm/Native.c +++ b/vm/Native.c @@ -114,7 +114,7 @@ void dvmResolveNativeMethod(const u4* args, JValue* pResult, if (func != NULL) { /* found it, point it at the JNI bridge and then call it */ dvmUseJNIBridge((Method*) method, func); - dvmCallJNIMethod(args, pResult, method, self); + (*method->nativeFunc)(args, pResult, method, self); return; } diff --git a/vm/Thread.h b/vm/Thread.h index 45018b4a0..1725b1edb 100644 --- a/vm/Thread.h +++ b/vm/Thread.h @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + /* * VM thread support. */ @@ -22,7 +23,7 @@ #include "jni.h" #if defined(CHECK_MUTEX) && !defined(__USE_UNIX98) -/* Linux lacks this unless you #define __USE_UNIX98 */ +/* glibc lacks this unless you #define __USE_UNIX98 */ int pthread_mutexattr_settype(pthread_mutexattr_t *attr, int type); enum { PTHREAD_MUTEX_ERRORCHECK = PTHREAD_MUTEX_ERRORCHECK_NP }; #endif @@ -151,7 +152,6 @@ typedef struct Thread { ReferenceTable internalLocalRefTable; /* JNI local reference tracking */ - // TODO: move this to JNIEnvExt to avoid an indirection? ReferenceTable jniLocalRefTable; /* JNI native monitor reference tracking (initialized on first use) */ diff --git a/vm/interp/Stack.c b/vm/interp/Stack.c index 969bea91c..838147fe1 100644 --- a/vm/interp/Stack.c +++ b/vm/interp/Stack.c @@ -463,6 +463,7 @@ void dvmCallMethodV(Thread* self, const Method* method, Object* obj, verifyCount++; } + JNIEnv* env = self->jniEnv; while (*desc != '\0') { switch (*(desc++)) { case 'D': case 'J': { @@ -479,16 +480,15 @@ void dvmCallMethodV(Thread* self, const Method* method, Object* obj, verifyCount++; break; } -#ifdef WITH_EXTRA_OBJECT_VALIDATION case 'L': { /* 'shorty' descr uses L for all refs, incl array */ - Object* argObj = (Object*) va_arg(args, u4); + void* argObj = va_arg(args, void*); assert(obj == NULL || dvmIsValidObject(obj)); - *ins++ = (u4) argObj; + *ins++ = (u4) dvmDecodeIndirectRef(env, argObj); verifyCount++; break; } -#endif default: { + /* Z B C S I -- all passed as 32-bit integers */ *ins++ = va_arg(args, u4); verifyCount++; break; @@ -558,56 +558,47 @@ void dvmCallMethodA(Thread* self, const Method* method, Object* obj, /* put "this" pointer into in0 if appropriate */ if (!dvmIsStaticMethod(method)) { assert(obj != NULL); - *ins++ = (u4) obj; + *ins++ = (u4) obj; /* obj is a "real" ref */ verifyCount++; } + JNIEnv* env = self->jniEnv; while (*desc != '\0') { - switch (*(desc++)) { - case 'D': case 'J': { - memcpy(ins, &args->j, 8); /* EABI prevents direct store */ - ins += 2; - verifyCount += 2; - args++; - break; - } - case 'F': case 'I': case 'L': { /* (no '[' in short signatures) */ - *ins++ = args->i; /* get all 32 bits */ - verifyCount++; - args++; - break; - } - case 'S': { - *ins++ = args->s; /* 16 bits, sign-extended */ - verifyCount++; - args++; - break; - } - case 'C': { - *ins++ = args->c; /* 16 bits, unsigned */ - verifyCount++; - args++; - break; - } - case 'B': { - *ins++ = args->b; /* 8 bits, sign-extended */ - verifyCount++; - args++; - break; - } - case 'Z': { - *ins++ = args->z; /* 8 bits, zero or non-zero */ - verifyCount++; - args++; - break; - } - default: { - LOGE("Invalid char %c in short signature of %s.%s\n", - *(desc-1), clazz->descriptor, method->name); - assert(false); - goto bail; - } + switch (*desc++) { + case 'D': /* 64-bit quantity; have to use */ + case 'J': /* memcpy() in case of mis-alignment */ + memcpy(ins, &args->j, 8); + ins += 2; + verifyCount++; /* this needs an extra push */ + break; + case 'L': /* includes array refs */ + *ins++ = (u4) dvmDecodeIndirectRef(env, args->l); + break; + case 'F': + case 'I': + *ins++ = args->i; /* full 32 bits */ + break; + case 'S': + *ins++ = args->s; /* 16 bits, sign-extended */ + break; + case 'C': + *ins++ = args->c; /* 16 bits, unsigned */ + break; + case 'B': + *ins++ = args->b; /* 8 bits, sign-extended */ + break; + case 'Z': + *ins++ = args->z; /* 8 bits, zero or non-zero */ + break; + default: + LOGE("Invalid char %c in short signature of %s.%s\n", + *(desc-1), clazz->descriptor, method->name); + assert(false); + goto bail; } + + verifyCount++; + args++; } #ifndef NDEBUG @@ -1158,11 +1149,15 @@ static void dumpFrames(const DebugOutputTarget* target, void* framePtr, first = false; - assert(framePtr != saveArea->prevFrame); + if (saveArea->prevFrame != NULL && saveArea->prevFrame <= framePtr) { + LOGW("Warning: loop in stack trace at frame %d (%p -> %p)\n", + checkCount, framePtr, saveArea->prevFrame); + break; + } framePtr = saveArea->prevFrame; checkCount++; - if (checkCount > 200) { + if (checkCount > 300) { dvmPrintDebugMessage(target, " ***** printed %d frames, not showing any more\n", checkCount); -- 2.11.0