[vmkit-commits] [PATCH] Touchup ClassLoader class-caching policies.

[Will Dietz]

Inlined below.

Refactor code that ensures a class is in a given classloader's
classmap into new method "ensureCached".

Additionally, drop some short-circuit logic that looks directly to
bootstrap loader and in a few places call ensureCached where we didn't
before.  Idea is to ensure that if a class is loaded through a
particular classloader that it should be cached there.

Also drop the redundant code in loadClassFromJavaString while we're at it :).

FWIW new behavior causes both runtimes to pass mauve's classloader
tests (neither did before), but of course this change should make
sense beyond that also.

~Will

>From d1d307c9e14a9a01d11f5dd382327ace5c9a8cd0 Mon Sep 17 00:00:00 2001
From: Will Dietz <w at wdtz.org>
Date: Tue, 29 Nov 2011 09:11:27 -0600
Subject: [PATCH 2/2] Touchup ClassLoader class-caching policies.

Essentially, if a class is loaded by a call to a particular classloader it
should be cached there.
---
 lib/j3/ClassLib/OpenJDK/OpenJDK.inc |    3 +-
 lib/j3/VMCore/JnjvmClassLoader.cpp  |   66 ++++++++++------------------------
 lib/j3/VMCore/JnjvmClassLoader.h    |    4 ++
 3 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/lib/j3/ClassLib/OpenJDK/OpenJDK.inc
b/lib/j3/ClassLib/OpenJDK/OpenJDK.inc
index 1b20596..f5e0c3c 100644
--- a/lib/j3/ClassLib/OpenJDK/OpenJDK.inc
+++ b/lib/j3/ClassLib/OpenJDK/OpenJDK.inc
@@ -1312,8 +1312,7 @@ JVM_FindLoadedClass(JNIEnv *env, jobject
_loader, jstring name) {
   CommonClass * Cl = 0;

   JCL = JnjvmClassLoader::getJnjvmLoaderFromJavaObject(loader, vm);
-  if (JCL)
-   Cl = JCL->lookupClassFromJavaString(str);
+  Cl = JCL->lookupClassFromJavaString(str);
   jclass res = Cl ? (jclass)Cl->getClassDelegateePtr(vm) : 0;
   RETURN_FROM_JNI(res);

diff --git a/lib/j3/VMCore/JnjvmClassLoader.cpp
b/lib/j3/VMCore/JnjvmClassLoader.cpp
index 2a77030..f8e5980 100644
--- a/lib/j3/VMCore/JnjvmClassLoader.cpp
+++ b/lib/j3/VMCore/JnjvmClassLoader.cpp
@@ -344,6 +344,12 @@ UserClass* JnjvmClassLoader::loadName(const UTF8*
name, bool doResolve,
     vm->noClassDefFoundError(name);
   }

+  ensureCached(cl);
+
+  return cl;
+}
+
+void JnjvmClassLoader::ensureCached(UserCommonClass* cl) {
   if (cl && cl->classLoader != this) {
     classes->lock.lock();
     ClassMap::iterator End = classes->map.end();
@@ -352,8 +358,6 @@ UserClass* JnjvmClassLoader::loadName(const UTF8*
name, bool doResolve,
       classes->map.insert(std::make_pair(cl->name, cl));
     classes->lock.unlock();
   }
-
-  return cl;
 }


@@ -414,6 +418,7 @@ UserCommonClass*
JnjvmClassLoader::lookupClassOrArray(const UTF8* name) {

   if (this != bootstrapLoader) {
     temp = bootstrapLoader->lookupClassOrArray(name);
+    ensureCached(temp);
     if (temp) return temp;
   }

@@ -450,6 +455,7 @@ UserCommonClass*
JnjvmClassLoader::loadClassFromUserUTF8(const UTF8* name,
     if (prim) return constructArray(name);
     if (componentName) {
       UserCommonClass* temp = loadName(componentName, doResolve,
doThrow, NULL);
+      ensureCached(temp);
       if (temp) return constructArray(name);
     }
   } else {
@@ -464,7 +470,6 @@ UserCommonClass*
JnjvmClassLoader::loadClassFromAsciiz(const char* asciiz,
                                                        bool doThrow) {
   const UTF8* name = hashUTF8->lookupAsciiz(asciiz);
   vmkit::ThreadAllocator threadAllocator;
-  UserCommonClass* result = NULL;
   if (!name) name = bootstrapLoader->hashUTF8->lookupAsciiz(asciiz);
   if (!name) {
     uint32 size = strlen(asciiz);
@@ -478,16 +483,6 @@ UserCommonClass*
JnjvmClassLoader::loadClassFromAsciiz(const char* asciiz,
     name = temp;
   }

-  result = lookupClass(name);
-  if ((result == NULL) && (this != bootstrapLoader)) {
-    result = bootstrapLoader->lookupClassOrArray(name);
-    if (result != NULL) {
-      if (result->isClass() && doResolve) {
-        result->asClass()->resolveClass();
-      }
-      return result;
-    }
-  }

   return loadClassFromUserUTF8(name, doResolve, doThrow, NULL);
 }
@@ -496,36 +491,22 @@ UserCommonClass*
JnjvmClassLoader::loadClassFromAsciiz(const char* asciiz,
 UserCommonClass*
 JnjvmClassLoader::loadClassFromJavaString(JavaString* str, bool doResolve,
                                           bool doThrow) {
-
+
   llvm_gcroot(str, 0);
   vmkit::ThreadAllocator allocator;
   UTF8* name = (UTF8*)allocator.Allocate(sizeof(UTF8) + str->count *
sizeof(uint16));
-
+
   name->size = str->count;
-  if (ArrayUInt16::getElement(JavaString::getValue(str), str->offset)
!= I_TAB) {
-    for (sint32 i = 0; i < str->count; ++i) {
-      uint16 cur = ArrayUInt16::getElement(JavaString::getValue(str),
str->offset + i);
-      if (cur == '.') name->elements[i] = '/';
-      else if (cur == '/') {
-        return 0;
-      }
-      else name->elements[i] = cur;
-    }
-  } else {
-    for (sint32 i = 0; i < str->count; ++i) {
-      uint16 cur = ArrayUInt16::getElement(JavaString::getValue(str),
str->offset + i);
-      if (cur == '.') {
-        name->elements[i] = '/';
-      } else if (cur == '/') {
-        return 0;
-      } else {
-        name->elements[i] = cur;
-      }
+  for (sint32 i = 0; i < str->count; ++i) {
+    uint16 cur = ArrayUInt16::getElement(JavaString::getValue(str),
str->offset + i);
+    if (cur == '.') name->elements[i] = '/';
+    else if (cur == '/') {
+      return 0;
     }
+    else name->elements[i] = cur;
   }
-
-  UserCommonClass* cls = loadClassFromUserUTF8(name, doResolve, doThrow, str);
-  return cls;
+
+  return loadClassFromUserUTF8(name, doResolve, doThrow, str);
 }

 UserCommonClass* JnjvmClassLoader::lookupClassFromJavaString(JavaString* str) {
@@ -585,15 +566,8 @@ UserClassArray*
JnjvmClassLoader::constructArray(const UTF8* name) {
   assert(cl && "no base class for an array");
   JnjvmClassLoader* ld = cl->classLoader;
   res = ld->constructArray(name, cl);
-
-  if (res && res->classLoader != this) {
-    classes->lock.lock();
-    ClassMap::iterator End = classes->map.end();
-    ClassMap::iterator I = classes->map.find(res->name);
-    if (I == End)
-      classes->map.insert(std::make_pair(res->name, res));
-    classes->lock.unlock();
-  }
+
+  ensureCached(res);
   return res;
 }

diff --git a/lib/j3/VMCore/JnjvmClassLoader.h b/lib/j3/VMCore/JnjvmClassLoader.h
index c0f8a4f..b3bce46 100644
--- a/lib/j3/VMCore/JnjvmClassLoader.h
+++ b/lib/j3/VMCore/JnjvmClassLoader.h
@@ -84,6 +84,10 @@ private:
   ///
   const UTF8* lookupComponentName(const UTF8* name, UTF8* holder, bool& prim);

+
+  /// ensureCached - Make sure the specified class is in our classes map
+  ///
+  void ensureCached(UserCommonClass* cl);
 protected:

   JnjvmClassLoader(vmkit::BumpPtrAllocator& Alloc) : allocator(Alloc) {}
-- 
1.7.5.1