# HG changeset patch # User coleenp # Date 1495109872 14400 # Node ID 75902cea18afbd4d3c722f27c113229c06af5d2a # Parent c872a196b75f64ce24de3fb0ef72c3804c651ab8 8166848: Performance bug: SystemDictionary - optimization Summary: Check instead that a bucket isn't 10x the average Reviewed-by: iklam, gziemski, sspitsyn diff -r c872a196b75f -r 75902cea18af hotspot/src/share/vm/classfile/dictionary.cpp --- a/hotspot/src/share/vm/classfile/dictionary.cpp Wed May 17 23:36:19 2017 +0200 +++ b/hotspot/src/share/vm/classfile/dictionary.cpp Thu May 18 08:17:52 2017 -0400 @@ -337,15 +337,12 @@ DictionaryEntry* Dictionary::get_entry(int index, unsigned int hash, Symbol* class_name, ClassLoaderData* loader_data) { - DEBUG_ONLY(_lookup_count++); for (DictionaryEntry* entry = bucket(index); entry != NULL; entry = entry->next()) { if (entry->hash() == hash && entry->equals(class_name, loader_data)) { - DEBUG_ONLY(bucket_count_hit(index)); return entry; } - DEBUG_ONLY(_lookup_length++); } return NULL; } @@ -548,114 +545,24 @@ tty->cr(); } -#ifdef ASSERT -void Dictionary::printPerformanceInfoDetails() { - if (log_is_enabled(Info, hashtables)) { - ResourceMark rm; - - log_info(hashtables)(" "); - log_info(hashtables)("Java system dictionary (table_size=%d, classes=%d)", - table_size(), number_of_entries()); - log_info(hashtables)("1st number: the bucket index"); - log_info(hashtables)("2nd number: the hit percentage for this bucket"); - log_info(hashtables)("3rd number: the entry's index within this bucket"); - log_info(hashtables)("4th number: the hash index of this entry"); - log_info(hashtables)(" "); - - // find top buckets with highest lookup count -#define TOP_COUNT 16 - int topItemsIndicies[TOP_COUNT]; - for (int i = 0; i < TOP_COUNT; i++) { - topItemsIndicies[i] = i; - } - double total = 0.0; - for (int i = 0; i < table_size(); i++) { - // find the total count number, so later on we can - // express bucket lookup count as a percentage of all lookups - unsigned value = bucket_hits(i); - total += value; - - // find the top entry with min value - int min_index = 0; - unsigned min_value = bucket_hits(topItemsIndicies[min_index]); - for (int j = 1; j < TOP_COUNT; j++) { - unsigned top_value = bucket_hits(topItemsIndicies[j]); - if (top_value < min_value) { - min_value = top_value; - min_index = j; - } - } - // if the bucket loookup value is bigger than the top buckets min - // move that bucket index into the top list - if (value > min_value) { - topItemsIndicies[min_index] = i; - } - } - - for (int index = 0; index < table_size(); index++) { - double percentage = 100.0 * (double)bucket_hits(index)/total; - int chain = 0; - for (DictionaryEntry* probe = bucket(index); - probe != NULL; - probe = probe->next()) { - Klass* e = probe->klass(); - ClassLoaderData* loader_data = probe->loader_data(); - bool is_defining_class = - (loader_data == e->class_loader_data()); - log_info(hashtables)("%4d: %5.2f%%: %3d: %10u: %s, loader %s", - index, percentage, chain, probe->hash(), e->external_name(), - (loader_data != NULL) ? loader_data->loader_name() : "NULL"); - - chain++; - } - if (chain == 0) { - log_info(hashtables)("%4d:", index+1); - } - } - log_info(hashtables)(" "); - - // print out the TOP_COUNT of buckets with highest lookup count (unsorted) - log_info(hashtables)("Top %d buckets:", TOP_COUNT); - for (int i = 0; i < TOP_COUNT; i++) { - log_info(hashtables)("%4d: hits %5.2f%%", - topItemsIndicies[i], - 100.0*(double)bucket_hits(topItemsIndicies[i])/total); - } - } +void DictionaryEntry::verify() { + Klass* e = klass(); + ClassLoaderData* cld = loader_data(); + guarantee(e->is_instance_klass(), + "Verify of system dictionary failed"); + // class loader must be present; a null class loader is the + // boostrap loader + guarantee(cld != NULL || DumpSharedSpaces || + cld->class_loader() == NULL || + cld->class_loader()->is_instance(), + "checking type of class_loader"); + e->verify(); + verify_protection_domain_set(); } -#endif // ASSERT void Dictionary::verify() { guarantee(number_of_entries() >= 0, "Verify of system dictionary failed"); - - int element_count = 0; - for (int index = 0; index < table_size(); index++) { - for (DictionaryEntry* probe = bucket(index); - probe != NULL; - probe = probe->next()) { - Klass* e = probe->klass(); - ClassLoaderData* loader_data = probe->loader_data(); - guarantee(e->is_instance_klass(), - "Verify of system dictionary failed"); - // class loader must be present; a null class loader is the - // boostrap loader - guarantee(loader_data != NULL || DumpSharedSpaces || - loader_data->class_loader() == NULL || - loader_data->class_loader()->is_instance(), - "checking type of class_loader"); - e->verify(); - probe->verify_protection_domain_set(); - element_count++; - } - } - guarantee(number_of_entries() == element_count, - "Verify of system dictionary failed"); -#ifdef ASSERT - if (!verify_lookup_length((double)number_of_entries() / table_size(), "System Dictionary")) { - this->printPerformanceInfoDetails(); - } -#endif // ASSERT - + verify_table("System Dictionary"); _pd_cache_table->verify(); } diff -r c872a196b75f -r 75902cea18af hotspot/src/share/vm/classfile/dictionary.hpp --- a/hotspot/src/share/vm/classfile/dictionary.hpp Wed May 17 23:36:19 2017 +0200 +++ b/hotspot/src/share/vm/classfile/dictionary.hpp Thu May 18 08:17:52 2017 -0400 @@ -211,6 +211,8 @@ } st->print_cr("pd set count = #%d", count); } + + void verify(); }; // Entry in a SymbolPropertyTable, mapping a single Symbol* diff -r c872a196b75f -r 75902cea18af hotspot/src/share/vm/classfile/moduleEntry.cpp --- a/hotspot/src/share/vm/classfile/moduleEntry.cpp Wed May 17 23:36:19 2017 +0200 +++ b/hotspot/src/share/vm/classfile/moduleEntry.cpp Thu May 18 08:17:52 2017 -0400 @@ -510,18 +510,7 @@ } void ModuleEntryTable::verify() { - int element_count = 0; - for (int i = 0; i < table_size(); i++) { - for (ModuleEntry* probe = bucket(i); - probe != NULL; - probe = probe->next()) { - probe->verify(); - element_count++; - } - } - guarantee(number_of_entries() == element_count, - "Verify of Module Entry Table failed"); - DEBUG_ONLY(verify_lookup_length((double)number_of_entries() / table_size(), "Module Entry Table")); + verify_table("Module Entry Table"); } void ModuleEntry::verify() { diff -r c872a196b75f -r 75902cea18af hotspot/src/share/vm/classfile/packageEntry.cpp --- a/hotspot/src/share/vm/classfile/packageEntry.cpp Wed May 17 23:36:19 2017 +0200 +++ b/hotspot/src/share/vm/classfile/packageEntry.cpp Thu May 18 08:17:52 2017 -0400 @@ -350,18 +350,7 @@ } void PackageEntryTable::verify() { - int element_count = 0; - for (int index = 0; index < table_size(); index++) { - for (PackageEntry* probe = bucket(index); - probe != NULL; - probe = probe->next()) { - probe->verify(); - element_count++; - } - } - guarantee(number_of_entries() == element_count, - "Verify of Package Entry Table failed"); - DEBUG_ONLY(verify_lookup_length((double)number_of_entries() / table_size(), "Package Entry Table")); + verify_table("Package Entry Table"); } void PackageEntry::verify() { diff -r c872a196b75f -r 75902cea18af hotspot/src/share/vm/classfile/protectionDomainCache.cpp --- a/hotspot/src/share/vm/classfile/protectionDomainCache.cpp Wed May 17 23:36:19 2017 +0200 +++ b/hotspot/src/share/vm/classfile/protectionDomainCache.cpp Thu May 18 08:17:52 2017 -0400 @@ -97,18 +97,7 @@ #endif void ProtectionDomainCacheTable::verify() { - int element_count = 0; - for (int index = 0; index < table_size(); index++) { - for (ProtectionDomainCacheEntry* probe = bucket(index); - probe != NULL; - probe = probe->next()) { - probe->verify(); - element_count++; - } - } - guarantee(number_of_entries() == element_count, - "Verify of protection domain cache table failed"); - DEBUG_ONLY(verify_lookup_length((double)number_of_entries() / table_size(), "Domain Cache Table")); + verify_table("Protection Domain Table"); } void ProtectionDomainCacheEntry::verify() { diff -r c872a196b75f -r 75902cea18af hotspot/src/share/vm/runtime/handles.hpp --- a/hotspot/src/share/vm/runtime/handles.hpp Wed May 17 23:36:19 2017 +0200 +++ b/hotspot/src/share/vm/runtime/handles.hpp Thu May 18 08:17:52 2017 -0400 @@ -126,6 +126,8 @@ // Metadata Handles. Unlike oop Handles these are needed to prevent metadata // from being reclaimed by RedefineClasses. +// Metadata Handles should be passed around as const references to avoid copy construction +// and destruction for parameters. // Specific Handles for different oop types #define DEF_METADATA_HANDLE(name, type) \ diff -r c872a196b75f -r 75902cea18af hotspot/src/share/vm/utilities/hashtable.cpp --- a/hotspot/src/share/vm/utilities/hashtable.cpp Wed May 17 23:36:19 2017 +0200 +++ b/hotspot/src/share/vm/utilities/hashtable.cpp Thu May 18 08:17:52 2017 -0400 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2016, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2017, 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 @@ -24,7 +24,11 @@ #include "precompiled.hpp" #include "classfile/altHashing.hpp" +#include "classfile/dictionary.hpp" #include "classfile/javaClasses.inline.hpp" +#include "classfile/moduleEntry.hpp" +#include "classfile/packageEntry.hpp" +#include "classfile/protectionDomainCache.hpp" #include "classfile/stringTable.hpp" #include "memory/allocation.inline.hpp" #include "memory/filemap.hpp" @@ -276,14 +280,22 @@ } -template void BasicHashtable::verify() { - int count = 0; - for (int i = 0; i < table_size(); i++) { - for (BasicHashtableEntry* p = bucket(i); p != NULL; p = p->next()) { - ++count; +template +template void BasicHashtable::verify_table(const char* table_name) { + int element_count = 0; + int max_bucket_count = 0; + for (int index = 0; index < table_size(); index++) { + int bucket_count = 0; + for (T* probe = (T*)bucket(index); probe != NULL; probe = probe->next()) { + probe->verify(); + bucket_count++; } + element_count += bucket_count; + max_bucket_count = MAX2(max_bucket_count, bucket_count); } - assert(count == number_of_entries(), "number of hashtable entries incorrect"); + guarantee(number_of_entries() == element_count, + "Verify of %s failed", table_name); + DEBUG_ONLY(verify_lookup_length(max_bucket_count, table_name)); } @@ -291,18 +303,12 @@ #ifdef ASSERT -template bool BasicHashtable::verify_lookup_length(double load, const char *table_name) { - if ((!_lookup_warning) && (_lookup_count != 0) - && ((double)_lookup_length / (double)_lookup_count > load * 2.0)) { - warning("Performance bug: %s lookup_count=%d " - "lookup_length=%d average=%lf load=%f", - table_name, _lookup_count, _lookup_length, - (double)_lookup_length / _lookup_count, load); - _lookup_warning = true; - - return false; - } - return true; +// Assert if the longest bucket is 10x longer than the average bucket size. +// Could change back to a warning, but warnings are not noticed. +template void BasicHashtable::verify_lookup_length(int max_bucket_count, const char *table_name) { + log_info(hashtables)("%s max bucket size %d element count %d table size %d", table_name, + max_bucket_count, _number_of_entries, _table_size); + assert (max_bucket_count < ((1 + number_of_entries()/table_size())*10), "Table is unbalanced"); } #endif @@ -344,3 +350,8 @@ template class BasicHashtable; #endif template class BasicHashtable; + +template void BasicHashtable::verify_table(char const*); +template void BasicHashtable::verify_table(char const*); +template void BasicHashtable::verify_table(char const*); +template void BasicHashtable::verify_table(char const*); diff -r c872a196b75f -r 75902cea18af hotspot/src/share/vm/utilities/hashtable.hpp --- a/hotspot/src/share/vm/utilities/hashtable.hpp Wed May 17 23:36:19 2017 +0200 +++ b/hotspot/src/share/vm/utilities/hashtable.hpp Thu May 18 08:17:52 2017 -0400 @@ -124,17 +124,9 @@ // Instance variable BasicHashtableEntry* _entry; -#ifdef ASSERT -private: - unsigned _hits; -public: - unsigned hits() { return _hits; } - void count_hit() { _hits++; } -#endif - public: // Accessing - void clear() { _entry = NULL; DEBUG_ONLY(_hits = 0); } + void clear() { _entry = NULL; } // The following methods use order access methods to avoid race // conditions in multiprocessor systems. @@ -179,10 +171,7 @@ protected: #ifdef ASSERT - bool _lookup_warning; - mutable int _lookup_count; - mutable int _lookup_length; - bool verify_lookup_length(double load, const char *table_name); + void verify_lookup_length(int max_bucket_count, const char *table_name); #endif void initialize(int table_size, int entry_size, int number_of_entries); @@ -232,16 +221,7 @@ int number_of_entries() { return _number_of_entries; } - void verify() PRODUCT_RETURN; - -#ifdef ASSERT - void bucket_count_hit(int i) const { - _buckets[i].count_hit(); - } - unsigned bucket_hits(int i) const { - return _buckets[i].hits(); - } -#endif + template void verify_table(const char* table_name) PRODUCT_RETURN; }; diff -r c872a196b75f -r 75902cea18af hotspot/src/share/vm/utilities/hashtable.inline.hpp --- a/hotspot/src/share/vm/utilities/hashtable.inline.hpp Wed May 17 23:36:19 2017 +0200 +++ b/hotspot/src/share/vm/utilities/hashtable.inline.hpp Thu May 18 08:17:52 2017 -0400 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2016, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2017, 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 @@ -64,11 +64,6 @@ _first_free_entry = NULL; _end_block = NULL; _number_of_entries = number_of_entries; -#ifdef ASSERT - _lookup_warning = false; - _lookup_count = 0; - _lookup_length = 0; -#endif }