# HG changeset patch # User hseigel # Date 1548340730 18000 # Node ID 525f212f1bda3ef5c33771e1530ed8764983448d # Parent 9459533ef916cd573d595f04ecee1a59d229cf8d 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 diff -r 9459533ef916 -r 525f212f1bda src/hotspot/share/classfile/classLoader.cpp --- 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); diff -r 9459533ef916 -r 525f212f1bda src/hotspot/share/classfile/moduleEntry.cpp --- 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::add_entry(index, (HashtableEntry*)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. diff -r 9459533ef916 -r 525f212f1bda src/hotspot/share/classfile/moduleEntry.hpp --- 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::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)); } diff -r 9459533ef916 -r 525f212f1bda src/hotspot/share/classfile/modules.cpp --- 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 diff -r 9459533ef916 -r 525f212f1bda src/hotspot/share/classfile/packageEntry.cpp --- 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::add_entry(index, (HashtableEntry*)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); } } diff -r 9459533ef916 -r 525f212f1bda src/hotspot/share/classfile/packageEntry.hpp --- 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::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. diff -r 9459533ef916 -r 525f212f1bda test/hotspot/jtreg/runtime/NMT/CheckForProperDetailStackTrace.java --- 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(