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

[David Holmes]

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
>