8217660: Refactor module related locked_create_entry_or_null() functions
authorhseigel
Thu, 24 Jan 2019 09:38:50 -0500
changeset 53471 525f212f1bda
parent 53470 9459533ef916
child 53472 cb43e14dc68b
8217660: Refactor module related locked_create_entry_or_null() functions Summary: Remove function return values and add functions that create entries without doing unneeded lookups. Reviewed-by: redestad, lfoltan
src/hotspot/share/classfile/classLoader.cpp
src/hotspot/share/classfile/moduleEntry.cpp
src/hotspot/share/classfile/moduleEntry.hpp
src/hotspot/share/classfile/modules.cpp
src/hotspot/share/classfile/packageEntry.cpp
src/hotspot/share/classfile/packageEntry.hpp
test/hotspot/jtreg/runtime/NMT/CheckForProperDetailStackTrace.java
--- a/src/hotspot/share/classfile/classLoader.cpp	Thu Jan 24 09:25:06 2019 -0500
+++ b/src/hotspot/share/classfile/classLoader.cpp	Thu Jan 24 09:38:50 2019 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -1724,7 +1724,7 @@
 
   {
     MutexLocker ml(Module_lock, THREAD);
-    ModuleEntry* jb_module = null_cld_modules->locked_create_entry_or_null(Handle(),
+    ModuleEntry* jb_module = null_cld_modules->locked_create_entry(Handle(),
                                false, vmSymbols::java_base(), NULL, NULL, null_cld);
     if (jb_module == NULL) {
       vm_exit_during_initialization("Unable to create ModuleEntry for " JAVA_BASE_NAME);
--- a/src/hotspot/share/classfile/moduleEntry.cpp	Thu Jan 24 09:25:06 2019 -0500
+++ b/src/hotspot/share/classfile/moduleEntry.cpp	Thu Jan 24 09:38:50 2019 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -398,23 +398,22 @@
   Hashtable<Symbol*, mtModule>::add_entry(index, (HashtableEntry<Symbol*, mtModule>*)new_entry);
 }
 
-ModuleEntry* ModuleEntryTable::locked_create_entry_or_null(Handle module_handle,
-                                                           bool is_open,
-                                                           Symbol* module_name,
-                                                           Symbol* module_version,
-                                                           Symbol* module_location,
-                                                           ClassLoaderData* loader_data) {
-  assert(module_name != NULL, "ModuleEntryTable locked_create_entry_or_null should never be called for unnamed module.");
+// Create an entry in the class loader's module_entry_table.  It is the
+// caller's responsibility to ensure that the entry has not already been
+// created.
+ModuleEntry* ModuleEntryTable::locked_create_entry(Handle module_handle,
+                                                   bool is_open,
+                                                   Symbol* module_name,
+                                                   Symbol* module_version,
+                                                   Symbol* module_location,
+                                                   ClassLoaderData* loader_data) {
+  assert(module_name != NULL, "ModuleEntryTable locked_create_entry should never be called for unnamed module.");
   assert(Module_lock->owned_by_self(), "should have the Module_lock");
-  // Check if module already exists.
-  if (lookup_only(module_name) != NULL) {
-    return NULL;
-  } else {
-    ModuleEntry* entry = new_entry(compute_hash(module_name), module_handle, is_open, module_name,
-                                   module_version, module_location, loader_data);
-    add_entry(index_for(module_name), entry);
-    return entry;
-  }
+  assert(lookup_only(module_name) == NULL, "Module already exists");
+  ModuleEntry* entry = new_entry(compute_hash(module_name), module_handle, is_open, module_name,
+                                 module_version, module_location, loader_data);
+  add_entry(index_for(module_name), entry);
+  return entry;
 }
 
 // lookup_only by Symbol* to find a ModuleEntry.
--- a/src/hotspot/share/classfile/moduleEntry.hpp	Thu Jan 24 09:25:06 2019 -0500
+++ b/src/hotspot/share/classfile/moduleEntry.hpp	Thu Jan 24 09:38:50 2019 -0500
@@ -236,14 +236,14 @@
     return (ModuleEntry*)Hashtable<Symbol*, mtModule>::bucket(i);
   }
 
-  // Create module in loader's module entry table, if already exists then
-  // return null.  Assume Module_lock has been locked by caller.
-  ModuleEntry* locked_create_entry_or_null(Handle module_handle,
-                                           bool is_open,
-                                           Symbol* module_name,
-                                           Symbol* module_version,
-                                           Symbol* module_location,
-                                           ClassLoaderData* loader_data);
+  // Create module in loader's module entry table.  Assume Module_lock
+  // has been locked by caller.
+  ModuleEntry* locked_create_entry(Handle module_handle,
+                                   bool is_open,
+                                   Symbol* module_name,
+                                   Symbol* module_version,
+                                   Symbol* module_location,
+                                   ClassLoaderData* loader_data);
 
   // Only lookup module within loader's module entry table.  The table read is lock-free.
   ModuleEntry* lookup_only(Symbol* name);
@@ -253,7 +253,10 @@
 
   // Special handling for java.base
   static ModuleEntry* javabase_moduleEntry()                   { return _javabase_module; }
-  static void set_javabase_moduleEntry(ModuleEntry* java_base) { _javabase_module = java_base; }
+  static void set_javabase_moduleEntry(ModuleEntry* java_base) {
+    assert(_javabase_module == NULL, "_javabase_module is already defined");
+    _javabase_module = java_base;
+  }
 
   static bool javabase_defined() { return ((_javabase_module != NULL) &&
                                            (_javabase_module->module() != NULL)); }
--- a/src/hotspot/share/classfile/modules.cpp	Thu Jan 24 09:25:06 2019 -0500
+++ b/src/hotspot/share/classfile/modules.cpp	Thu Jan 24 09:38:50 2019 -0500
@@ -208,10 +208,9 @@
       // loop through and add any new packages for java.base
       for (int x = 0; x < pkg_list->length(); x++) {
         // Some of java.base's packages were added early in bootstrapping, ignore duplicates.
-        PackageEntry* pkg =
-          package_table->locked_create_entry_or_null(pkg_list->at(x),
-                                                     ModuleEntryTable::javabase_moduleEntry());
-        assert(pkg != NULL || package_table->locked_lookup_only(pkg_list->at(x)) != NULL,
+        package_table->locked_create_entry_if_not_exist(pkg_list->at(x),
+                                                        ModuleEntryTable::javabase_moduleEntry());
+        assert(package_table->locked_lookup_only(pkg_list->at(x)) != NULL,
                "Unable to create a " JAVA_BASE_NAME " package entry");
         // Unable to have a GrowableArray of TempNewSymbol.  Must decrement the refcount of
         // the Symbol* that was created above for each package. The refcount was incremented
@@ -402,20 +401,17 @@
 
     // Add the module and its packages.
     if (!dupl_modules && existing_pkg == NULL) {
-      // Create the entry for this module in the class loader's module entry table.
-      ModuleEntry* module_entry = module_table->locked_create_entry_or_null(module_handle,
+      if (module_table->lookup_only(module_symbol) == NULL) {
+        // Create the entry for this module in the class loader's module entry table.
+        ModuleEntry* module_entry = module_table->locked_create_entry(module_handle,
                                     (is_open == JNI_TRUE), module_symbol,
                                     version_symbol, location_symbol, loader_data);
+        assert(module_entry != NULL, "module_entry creation failed");
 
-      if (module_entry == NULL) {
-        dupl_modules = true;
-      } else {
         // Add the packages.
         assert(pkg_list->length() == 0 || package_table != NULL, "Bad package table");
-        PackageEntry* pkg;
         for (int y = 0; y < pkg_list->length(); y++) {
-          pkg = package_table->locked_create_entry_or_null(pkg_list->at(y), module_entry);
-          assert(pkg != NULL, "Unable to create a module's package entry");
+          package_table->locked_create_entry(pkg_list->at(y), module_entry);
 
           // Unable to have a GrowableArray of TempNewSymbol.  Must decrement the refcount of
           // the Symbol* that was created above for each package. The refcount was incremented
@@ -425,6 +421,8 @@
 
         // Store pointer to ModuleEntry record in java.lang.Module object.
         java_lang_Module::set_module_entry(module_handle(), module_entry);
+      } else {
+         dupl_modules = true;
       }
     }
   }  // Release the lock
--- a/src/hotspot/share/classfile/packageEntry.cpp	Thu Jan 24 09:25:06 2019 -0500
+++ b/src/hotspot/share/classfile/packageEntry.cpp	Thu Jan 24 09:38:50 2019 -0500
@@ -211,17 +211,22 @@
   Hashtable<Symbol*, mtModule>::add_entry(index, (HashtableEntry<Symbol*, mtModule>*)new_entry);
 }
 
-// Create package in loader's package entry table and return the entry.
-// If entry already exists, return null.  Assume Module lock was taken by caller.
-PackageEntry* PackageEntryTable::locked_create_entry_or_null(Symbol* name, ModuleEntry* module) {
+// Create package entry in loader's package entry table.  Assume Module lock
+// was taken by caller.
+void PackageEntryTable::locked_create_entry(Symbol* name, ModuleEntry* module) {
   assert(Module_lock->owned_by_self(), "should have the Module_lock");
-  // Check if package already exists.  Return NULL if it does.
-  if (locked_lookup_only(name) != NULL) {
-    return NULL;
-  } else {
-    PackageEntry* entry = new_entry(compute_hash(name), name, module);
-    add_entry(index_for(name), entry);
-    return entry;
+  assert(locked_lookup_only(name) == NULL, "Package entry already exists");
+  PackageEntry* entry = new_entry(compute_hash(name), name, module);
+  add_entry(index_for(name), entry);
+}
+
+// Create package entry in loader's package entry table if it does not already
+// exist.  Assume Module lock was taken by caller.
+void PackageEntryTable::locked_create_entry_if_not_exist(Symbol* name, ModuleEntry* module) {
+  assert(Module_lock->owned_by_self(), "should have the Module_lock");
+  // Check if package entry already exists.  If not, create it.
+  if (locked_lookup_only(name) == NULL) {
+    locked_create_entry(name, module);
   }
 }
 
--- a/src/hotspot/share/classfile/packageEntry.hpp	Thu Jan 24 09:25:06 2019 -0500
+++ b/src/hotspot/share/classfile/packageEntry.hpp	Thu Jan 24 09:38:50 2019 -0500
@@ -246,9 +246,13 @@
     return (PackageEntry*)Hashtable<Symbol*, mtModule>::bucket(i);
   }
 
-  // Create package in loader's package entry table and return the entry.
-  // If entry already exists, return null.  Assume Module lock was taken by caller.
-  PackageEntry* locked_create_entry_or_null(Symbol* name, ModuleEntry* module);
+  // Create package entry in loader's package entry table.  Assume Module
+  // lock was taken by caller.
+  void locked_create_entry(Symbol* name, ModuleEntry* module);
+
+  // Create package entry in loader's package entry table if it does not
+  // already exist.  Assume Module lock was taken by caller.
+  void locked_create_entry_if_not_exist(Symbol* name, ModuleEntry* module);
 
   // Lookup Package with loader's package entry table, add it if not found.
   // This will acquire the Module lock.
--- a/test/hotspot/jtreg/runtime/NMT/CheckForProperDetailStackTrace.java	Thu Jan 24 09:25:06 2019 -0500
+++ b/test/hotspot/jtreg/runtime/NMT/CheckForProperDetailStackTrace.java	Thu Jan 24 09:38:50 2019 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -42,7 +42,7 @@
     public static String stackTraceDefault =
         ".*Hashtable.*allocate_new_entry.*\n" +
         ".*ModuleEntryTable.*new_entry.*\n" +
-        ".*ModuleEntryTable.*locked_create_entry_or_null.*\n" +
+        ".*ModuleEntryTable.*locked_create_entry.*\n" +
         ".*Modules.*define_module.*\n";
 
     /* The stack trace we look for on Solaris and Windows slowdebug builds. For some
@@ -50,12 +50,12 @@
     public static String stackTraceAllocateHeap =
         ".*AllocateHeap.*\n" +
         ".*ModuleEntryTable.*new_entry.*\n" +
-        ".*ModuleEntryTable.*locked_create_entry_or_null.*\n" +
+        ".*ModuleEntryTable.*locked_create_entry.*\n" +
         ".*Modules.*define_module.*\n";
 
     /* A symbol that should always be present in NMT detail output. */
     private static String expectedSymbol =
-        "locked_create_entry_or_null";
+        "locked_create_entry";
 
     public static void main(String args[]) throws Exception {
         ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(