Code and Design Feedback request: 7114376: tune system dictionary size

[Karen Kinnear]

Folks,

Based on your helpful feedback, and help with benchmarking, I have redone the flags and resizing heuristics.
Webrev updated at:
    http://cr.openjdk.java.net/~acorn/7114376.01/webrev/

New flag: -XX:PredictedLoadedClassCount=#

I've left just the one flag - users can start with their actual loaded class count, but they can set that
number to whatever gives them the best performance. 

Testing:

Passed nsks (which included SA), jcks, jprt (which included CDS).
No performance difference for specjbb2005 on linux-amd64. 
volano2 appears to run 13% faster on linux-amd64 with dynamic resizing.
Large application runs in progress to determine initial tuning for default depth, so that may change.

thanks,
Karen

p.s. I filed 7133093 for suggestions on future improvements - thank you for those

On Jan 22, 2012, at 8:50 PM, David Holmes wrote:

> Hi Karen,
> 
> On 22/01/2012 5:04 AM, Karen Kinnear wrote:
>> Thank you for asking, my mistake. Here is an accessible webrev.
>> 
>> Webrev: http://cr.openjdk.java.net/~acorn/7114376/webrev/
> 
> systemDictionary.cpp:
> 
> I notice that in systemDictionary the calculation of the index has been moved inside the locked regions. This impacted some of the SD method signatures, depending on whether they grabbed the lock internally, or whether the lock was first acquired externally. Seems to me we could avoid a lot of duplicate index calculating code if we went a step further and changed all the methods to only take the hash and do their own index calculation if needed - in particular if find_class did the calculation then I think most of the other index calculations would disappear.
> 
> 1757 // Check if system dictionary should be resized to speed up find() calls
> 1758 // Default is to check on class unloading
> 1759 // SystemDictionarySizeIndex flag allows starting with a larger initial size
> 1760 // if SystemDictionaryAverageDepth is non-zero, also check on safepoint cleanup
> 1761 // and allow customers to select acceptable average depth to pass
> 1762 // to verify_lookup_length, otherwise use number_of_classes/table_size
> 1763 // verify_lookup_length compares actual average lookup depth: lookup_length/lookup_count
> 1764 // with theoretical or user-specified.
> 1765 // In future we may need to periodically calculate lookup_length to see
> 1766 // if we need to force a safepoint to resize system dictionary
> 1767 void SystemDictionary::should_resize_dictionary(int unloaded_count) {
> 
> I found the above comment block difficult to parse - perhaps some Capitals missing to indicate new points/sentences?
> 
> Also should this method assert that either the SD lock is held or else we're at a safepoint?
> 
> ---
> 
> dictionary.hpp/cpp
> 
> +   // Change size of system dictionary
> +   // Initially for growing when lookup time takes too long
> +   Dictionary* dictionary_resize(int newsize);
> 
> Is "system dictionary" distinct from SystemDictionary?
> 
> I don't like having this as an instance method as it doesn't resize the current dictionary but returns a new-one, leaving the caller to free the original, yet has the side-effect of clearing all entries from the current one. Seems to me we should simply have the caller do:
> 
> Dictionary* newdict = new Dictionary(newsize);
> olddict->copy_all_entries(newdict);
> delete olddict;
> olddict = newdict;
> 
> though perhaps copy_all_entries should really be move_all_entries?
> 
> 477 // This routine does not lock the system dictionary.
> ...
> 487 // Actually, the locking here is a bit trickier:
> ...
> 493 DictionaryEntry* Dictionary::get_entry(int index, unsigned int hash,
> 494                                        Symbol* class_name,
> 495                                        Handle class_loader) {
> 496   oop loader = class_loader();
> 497   _lookup_count++;
> 498
> 499   for (DictionaryEntry* entry = bucket(index);
> 500                         entry != NULL;
> 501                         entry = entry->next()) {
> 502     if (entry->hash() == hash && entry->equals(class_name, loader)) {
> 503       return entry;
> 504     }
> 505     _lookup_length++;
> 
> Your now non-debug counters seem to be being updated in a non-threadsafe manner - or are we assuming the lock has been taken in the caller?
> 
>> An alternative possibility for supplying a single flag might be: -XX:LoadedClasses=#
>> 
>> This could be an anticipated count of expected total classes, which we could use
>> internally to set the initial SystemDictionary size. This might outlast any internal changes
>> in data structures.
> 
> I think this might be a simpler starting point for things. Dynamically resizing the SD is transiently a bad thing for everyone:
> - pause while it happens
> - excessive memory use while it happens
> 
> better in my opinion to try to get the initial number of buckets right and not dynamically resize.
> 
> Cheers,
> David
> -----
> 
>> Tests run:
>> vmsqe: nsk.quick.testlist: solaris-sparc, fastdbg - this includes sa testing
>> jck: vm,lang,api: solaris-sparc, fastdbg
>> JPRT - this includes CDS testing
>> performance runs in progress, including frequency of checking need to resize
>> 
>> thanks,
>> Karen
>> 
>> On Jan 21, 2012, at 12:04 PM, David Schlosnagle wrote:
>> 
>>> On Fri, Jan 20, 2012 at 6:06 PM, Karen Kinnear<karen.kinnear at oracle.com>  wrote:
>>>> Please review initial proposal for: 7114376: tune system dictionary size
>>>> 
>>>> http://oklahoma.us.oracle.com/~kmkinnea/webrev/sdresize/webrev/
>>> 
>>> Karen,
>>> 
>>> Is there a URL where this webrev is accessible outside of Oracle?
>>> 
>>> Thanks,
>>> Dave
>>