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

[Karen Kinnear]

David,

Thank you for the detailed feedback. More below.

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.

Great suggestion - I will do that and send for re-review.
> 
> 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?
I am going to simplify the flags to just -XX:LoadedClasses=# so this comment will be rewritten and
sent out for re-review.
> 
> Also should this method assert that either the SD lock is held or else we're at a safepoint?
Will add the assertion that we are 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?
I will make that change. dictionary_resize used to do a lot more work, but the way it turned out
your suggestion is much better.
move_all_entries is a better name.
> 
> 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?
Good catch. This is called without a lock. 
So the question is - should I make this an atomic update, or since these are approximations used for heuristics
only, add a comment that we actually don't need them to be completely accurate? I can measure
cost of the atomic update.
> 
>> 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.
I believe we need both. It is simpler to just start with a better size - for now I will add the -XX:LoadedClasses=#
and use that as a starting point, and the only new flag, for customers that want to add a command-line argument.

Our goal is to have good performance without requiring custom tuning, so the dynamic resizing is valuable for
customers who do not want to figure out their usage and do not want to add a command-line argument.
I will simplify the ergonomic tuning to be based on the dynamically calculated LoadedClasses  as well, so
that if we do resize based on slower lookup times, that we resize to the current need rather than incrementally.

I expect the resizing to be beneficial both for customers with high numbers of loaded classes and I expect
the embedded customers to take advantage of resizing to potentially shrink the system dictionary. And I tried
to add the logic so that this could be reused for resizing other internal metadata structures.
> 
> Cheers,
> David
> -----

thank you for the very helpful feedback,
Karen

> 
>> 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
>>