8062116: JVMTI GetClassMethods is Slow

Allocate enough space for all jmethodids; make adding a jmethodid O(1)

Reviewed-by: coleenp, rasbold, sspitsyn

hotspot/src/share/vm/oops/instanceKlass.cpp

@@ -1730,6 +1730,25 @@ jmethodID InstanceKlass::get_jmethod_id(instanceKlassHandle ik_h, methodHandle m
   return id;
 }
 
+// Figure out how many jmethodIDs haven't been allocated, and make
+// sure space for them is pre-allocated.  This makes getting all
+// method ids much, much faster with classes with more than 8
+// methods, and has a *substantial* effect on performance with jvmti
+// code that loads all jmethodIDs for all classes.
+void InstanceKlass::ensure_space_for_methodids(int start_offset) {
+  int new_jmeths = 0;
+  int length = methods()->length();
+  for (int index = start_offset; index < length; index++) {
+    Method* m = methods()->at(index);
+    jmethodID id = m->find_jmethod_id_or_null();
+    if (id == NULL) {
+      new_jmeths++;
+    }
+  }
+  if (new_jmeths != 0) {
+    Method::ensure_jmethod_ids(class_loader_data(), new_jmeths);
+  }
+}
 
 // Common code to fetch the jmethodID from the cache or update the
 // cache with the new jmethodID. This function should never do anything

hotspot/src/share/vm/oops/instanceKlass.hpp

@@ -698,6 +698,7 @@ class InstanceKlass: public Klass {
                      jmethodID** to_dealloc_jmeths_p);
   static void get_jmethod_id_length_value(jmethodID* cache, size_t idnum,
                 size_t *length_p, jmethodID* id_p);
+  void ensure_space_for_methodids(int start_offset = 0);
   jmethodID jmethod_id_or_null(Method* method);
 
   // annotations support

hotspot/src/share/vm/oops/method.cpp

@@ -1727,59 +1727,98 @@ void BreakpointInfo::clear(Method* method) {
 // jmethodID handling
 
 // This is a block allocating object, sort of like JNIHandleBlock, only a
-// lot simpler.  There aren't many of these, they aren't long, they are rarely
-// deleted and so we can do some suboptimal things.
+// lot simpler.
 // It's allocated on the CHeap because once we allocate a jmethodID, we can
 // never get rid of it.
-// It would be nice to be able to parameterize the number of methods for
-// the null_class_loader but then we'd have to turn this and ClassLoaderData
-// into templates.
 
-// I feel like this brain dead class should exist somewhere in the STL
+static const int min_block_size = 8;
+
+class JNIMethodBlockNode : public CHeapObj<mtClass> {
+  friend class JNIMethodBlock;
+  Method**        _methods;
+  int             _number_of_methods;
+  int             _top;
+  JNIMethodBlockNode* _next;
+
+ public:
+
+  JNIMethodBlockNode(int num_methods = min_block_size);
+
+  ~JNIMethodBlockNode() { FREE_C_HEAP_ARRAY(Method*, _methods, mtInternal); }
+
+  void ensure_methods(int num_addl_methods) {
+    if (_top < _number_of_methods) {
+      num_addl_methods -= _number_of_methods - _top;
+      if (num_addl_methods <= 0) {
+        return;
+      }
+    }
+    if (_next == NULL) {
+      _next = new JNIMethodBlockNode(MAX2(num_addl_methods, min_block_size));
+    } else {
+      _next->ensure_methods(num_addl_methods);
+    }
+  }
+};
 
 class JNIMethodBlock : public CHeapObj<mtClass> {
-  enum { number_of_methods = 8 };
-
-  Method*         _methods[number_of_methods];
-  int             _top;
-  JNIMethodBlock* _next;
+  JNIMethodBlockNode _head;
+  JNIMethodBlockNode *_last_free;
  public:
   static Method* const _free_method;
 
-  JNIMethodBlock() : _next(NULL), _top(0) {
-    for (int i = 0; i< number_of_methods; i++) _methods[i] = _free_method;
+  JNIMethodBlock(int initial_capacity = min_block_size)
+      : _head(initial_capacity), _last_free(&_head) {}
+
+  void ensure_methods(int num_addl_methods) {
+    _last_free->ensure_methods(num_addl_methods);
   }
 
   Method** add_method(Method* m) {
-    if (_top < number_of_methods) {
-      // top points to the next free entry.
-      int i = _top;
-      _methods[i] = m;
-      _top++;
-      return &_methods[i];
-    } else if (_top == number_of_methods) {
-      // if the next free entry ran off the block see if there's a free entry
-      for (int i = 0; i< number_of_methods; i++) {
-        if (_methods[i] == _free_method) {
-          _methods[i] = m;
-          return &_methods[i];
+    for (JNIMethodBlockNode* b = _last_free; b != NULL; b = b->_next) {
+      if (b->_top < b->_number_of_methods) {
+        // top points to the next free entry.
+        int i = b->_top;
+        b->_methods[i] = m;
+        b->_top++;
+        _last_free = b;
+        return &(b->_methods[i]);
+      } else if (b->_top == b->_number_of_methods) {
+        // if the next free entry ran off the block see if there's a free entry
+        for (int i = 0; i < b->_number_of_methods; i++) {
+          if (b->_methods[i] == _free_method) {
+            b->_methods[i] = m;
+            _last_free = b;
+            return &(b->_methods[i]);
+          }
         }
+        // Only check each block once for frees.  They're very unlikely.
+        // Increment top past the end of the block.
+        b->_top++;
+      }
+      // need to allocate a next block.
+      if (b->_next == NULL) {
+        b->_next = _last_free = new JNIMethodBlockNode();
       }
-      // Only check each block once for frees.  They're very unlikely.
-      // Increment top past the end of the block.
-      _top++;
     }
-    // need to allocate a next block.
-    if (_next == NULL) {
-      _next = new JNIMethodBlock();
-    }
-    return _next->add_method(m);
+    guarantee(false, "Should always allocate a free block");
+    return NULL;
   }
 
   bool contains(Method** m) {
-    for (JNIMethodBlock* b = this; b != NULL; b = b->_next) {
-      for (int i = 0; i< number_of_methods; i++) {
-        if (&(b->_methods[i]) == m) {
+    if (m == NULL) return false;
+    for (JNIMethodBlockNode* b = &_head; b != NULL; b = b->_next) {
+      if (b->_methods <= m && m < b->_methods + b->_number_of_methods) {
+        // This is a bit of extra checking, for two reasons.  One is
+        // that contains() deals with pointers that are passed in by
+        // JNI code, so making sure that the pointer is aligned
+        // correctly is valuable.  The other is that <= and > are
+        // technically not defined on pointers, so the if guard can
+        // pass spuriously; no modern compiler is likely to make that
+        // a problem, though (and if one did, the guard could also
+        // fail spuriously, which would be bad).
+        ptrdiff_t idx = m - b->_methods;
+        if (b->_methods + idx == m) {
           return true;
         }
       }
@@ -1798,9 +1837,9 @@ class JNIMethodBlock : public CHeapObj<mtClass> {
   // During class unloading the methods are cleared, which is different
   // than freed.
   void clear_all_methods() {
-    for (JNIMethodBlock* b = this; b != NULL; b = b->_next) {
-      for (int i = 0; i< number_of_methods; i++) {
-        _methods[i] = NULL;
+    for (JNIMethodBlockNode* b = &_head; b != NULL; b = b->_next) {
+      for (int i = 0; i< b->_number_of_methods; i++) {
+        b->_methods[i] = NULL;
       }
     }
   }
@@ -1808,9 +1847,9 @@ class JNIMethodBlock : public CHeapObj<mtClass> {
   int count_methods() {
     // count all allocated methods
     int count = 0;
-    for (JNIMethodBlock* b = this; b != NULL; b = b->_next) {
-      for (int i = 0; i< number_of_methods; i++) {
-        if (_methods[i] != _free_method) count++;
+    for (JNIMethodBlockNode* b = &_head; b != NULL; b = b->_next) {
+      for (int i = 0; i< b->_number_of_methods; i++) {
+        if (b->_methods[i] != _free_method) count++;
       }
     }
     return count;
@@ -1821,6 +1860,36 @@ class JNIMethodBlock : public CHeapObj<mtClass> {
 // Something that can't be mistaken for an address or a markOop
 Method* const JNIMethodBlock::_free_method = (Method*)55;
 
+JNIMethodBlockNode::JNIMethodBlockNode(int num_methods) : _next(NULL), _top(0) {
+  _number_of_methods = MAX2(num_methods, min_block_size);
+  _methods = NEW_C_HEAP_ARRAY(Method*, _number_of_methods, mtInternal);
+  for (int i = 0; i < _number_of_methods; i++) {
+    _methods[i] = JNIMethodBlock::_free_method;
+  }
+}
+
+void Method::ensure_jmethod_ids(ClassLoaderData* loader_data, int capacity) {
+  ClassLoaderData* cld = loader_data;
+  if (!SafepointSynchronize::is_at_safepoint()) {
+    // Have to add jmethod_ids() to class loader data thread-safely.
+    // Also have to add the method to the list safely, which the cld lock
+    // protects as well.
+    MutexLockerEx ml(cld->metaspace_lock(),  Mutex::_no_safepoint_check_flag);
+    if (cld->jmethod_ids() == NULL) {
+      cld->set_jmethod_ids(new JNIMethodBlock(capacity));
+    } else {
+      cld->jmethod_ids()->ensure_methods(capacity);
+    }
+  } else {
+    // At safepoint, we are single threaded and can set this.
+    if (cld->jmethod_ids() == NULL) {
+      cld->set_jmethod_ids(new JNIMethodBlock(capacity));
+    } else {
+      cld->jmethod_ids()->ensure_methods(capacity);
+    }
+  }
+}
+
 // Add a method id to the jmethod_ids
 jmethodID Method::make_jmethod_id(ClassLoaderData* loader_data, Method* m) {
   ClassLoaderData* cld = loader_data;

hotspot/src/share/vm/oops/method.hpp

@@ -729,6 +729,11 @@ class Method : public Metadata {
   static jmethodID make_jmethod_id(ClassLoaderData* loader_data, Method* mh);
   static void destroy_jmethod_id(ClassLoaderData* loader_data, jmethodID mid);
 
+  // Ensure there is enough capacity in the internal tracking data
+  // structures to hold the number of jmethodIDs you plan to generate.
+  // This saves substantial time doing allocations.
+  static void ensure_jmethod_ids(ClassLoaderData* loader_data, int capacity);
+
   // Use resolve_jmethod_id() in situations where the caller is expected
   // to provide a valid jmethodID; the only sanity checks are in asserts;
   // result guaranteed not to be NULL.

hotspot/src/share/vm/prims/jvmtiEnv.cpp

@@ -2263,6 +2263,8 @@ JvmtiEnv::GetClassMethods(oop k_mirror, jint* method_count_ptr, jmethodID** meth
   int result_length = instanceK_h->methods()->length();
   jmethodID* result_list = (jmethodID*)jvmtiMalloc(result_length * sizeof(jmethodID));
   int index;
+  bool jmethodids_found = true;
+
   if (JvmtiExport::can_maintain_original_method_order()) {
     // Use the original method ordering indices stored in the class, so we can emit
     // jmethodIDs in the order they appeared in the class file
@@ -2270,14 +2272,40 @@ JvmtiEnv::GetClassMethods(oop k_mirror, jint* method_count_ptr, jmethodID** meth
       Method* m = instanceK_h->methods()->at(index);
       int original_index = instanceK_h->method_ordering()->at(index);
       assert(original_index >= 0 && original_index < result_length, "invalid original method index");
-      jmethodID id = m->jmethod_id();
+      jmethodID id;
+      if (jmethodids_found) {
+        id = m->find_jmethod_id_or_null();
+        if (id == NULL) {
+          // If we find an uninitialized value, make sure there is
+          // enough space for all the uninitialized values we might
+          // find.
+          instanceK_h->ensure_space_for_methodids(index);
+          jmethodids_found = false;
+          id = m->jmethod_id();
+        }
+      } else {
+        id = m->jmethod_id();
+      }
       result_list[original_index] = id;
     }
   } else {
     // otherwise just copy in any order
     for (index = 0; index < result_length; index++) {
       Method* m = instanceK_h->methods()->at(index);
-      jmethodID id = m->jmethod_id();
+      jmethodID id;
+      if (jmethodids_found) {
+        id = m->find_jmethod_id_or_null();
+        if (id == NULL) {
+          // If we find an uninitialized value, make sure there is
+          // enough space for all the uninitialized values we might
+          // find.
+          instanceK_h->ensure_space_for_methodids(index);
+          jmethodids_found = false;
+          id = m->jmethod_id();
+        }
+      } else {
+        id = m->jmethod_id();
+      }
       result_list[index] = id;
     }
   }