RFR: 8184765: Dynamically resize SystemDictionary

[David Holmes]

Thanks for the additional background Coleen!

David

On 14/10/2017 12:21 AM, coleen.phillimore at oracle.com wrote:
> 
> 
> On 10/13/17 9:11 AM, David Holmes wrote:
>> Hi Karen,
>>
>> On 13/10/2017 8:42 PM, Karen Kinnear wrote:
>>> A quick note - we are resizing the hash table, i.e. number of linked 
>>> lists, so as to speed up lookup if it is too small, or waste less 
>>> space if it is
>>> too large. Each dictionary may have multiple entries off of it - one 
>>> per loaded class - so there is no such thing as running out
>>> of space based on the sizing (just based on total OOM). I have not 
>>> yet looked at the change - but just wanted to clarify the
>>> goal and concept for folks first.
>>
>> Thanks for clarifying - though AFAICS this proposal only grows the 
>> dictionary. Is the "waste less space" to be achieved by starting with 
>> a smaller dictionary initially? Do we have a way to control the 
>> initial size?
> 
> I made the change so that there is one dictionary per class loader 
> data.  The <bootloader> dictionary starts at 1009 and from what I've 
> seen in testing has about 900 entries in it on average. So seems 
> appropriately sized.  The application class loader dictionary (as 
> default) or dictionary defined with -Djava.system.class.loader and the 
> other dictionaries start at 107.   The latter can be started with a 
> larger size with -XX:+PredictedLoadedClassCount.   We had trouble 
> finding any users who set this option.
> 
> The application class loader dictionary is hard to find an initial size 
> for, and rather than have the user guess which size they want, we will 
> do automatic resizing to make it larger.  I think the initial size of 
> application class loader dictionary should be more like 1009.  It was a 
> bug that I initialized it with 107 actually.
> 
> We do not have a way to specify initial size for class loader 
> dictionaries.   And we don't resize down because that doesn't make any 
> sense.   Entries are almost never deleted(*), rather the whole 
> dictionary goes away when the class is unloaded.
> 
> So this change is mostly for a class loader that loads a huge number of 
> classes, that might experience slower than average lookups.  We do have 
> a user who does this internally.
> 
> Coleen
> 
>> Thanks,
>> David
>>
>>> thanks
>>> Karen
>>>
>>>> On Oct 11, 2017, at 4:10 PM, coleen.phillimore at oracle.com wrote:
>>>>
>>>>
>>>> Gerard, some preliminary comments.
>>>>
>>>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html 
>>>>
>>>>
>>>> *+{*
>>>> *!_MutexLocker mu(SystemDictionary_lock, THREAD_);*
>>>> *!_Klass* probe = dictionary->find(_d_hash, name, protection_domain);*
>>>> *if (probe != NULL) return probe;*
>>>> **
>>>> *+ }*
>>>>
>>>> I don't think you need this because dictionary->find() should have 
>>>> the NoSafepointVerifier, so the index will not change.
>>>>
>>>> http://cr.openjdk.java.net/~gziemski/8184765_rev1/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html 
>>>>
>>>>
>>>> I think we want a global _some_dictionary_needs_resizing to avoid 
>>>> walking through the CLDG.
>>>>
>>>> And have Dictionary have a field _resizing_needed to avoid the 
>>>> calculation during the safepoint, which is set when adding an entry.
>>>>
>>>> _resizing_needed = *(number_of_entries() > 
>>>> (_resize_load_trigger*table_size());
>>>> *CLDG::_any_resizing_needed |= _resizing_needed;   // or something 
>>>> like that.
>>>>
>>>> I can write more about the rationale of this change in the bug 
>>>> report, if needed.
>>>>
>>>> Thank you for doing this change.
>>>> Coleen
>>>>
>>>>
>>>> On 10/10/17 4:40 PM, Gerard Ziemski wrote:
>>>>> hi all,
>>>>>
>>>>> Please review this change that adds dynamic resizing of system 
>>>>> dictionaries.
>>>>>
>>>>> The biggest change is refactoring of the code that protects 
>>>>> calculating of the table entry’s index using SystemDictionary_lock
>>>>>
>>>>> A few notes:
>>>>>
>>>>> - dynamic resizing is off when either dumping or using shared spaces
>>>>> - we remove the experimental runtime option 
>>>>> “PredictedLoadedClassCount” and add 
>>>>> “DynamicallyResizeSystemDictionaries” to turn the resizing off 
>>>>> (it’s on by default)
>>>>> - the jtreg test uses stream of bytes to dynamically load numbered 
>>>>> classes from memory instead of disk (thank you Ioi)
>>>>>
>>>>> bug:    https://bugs.openjdk.java.net/browse/JDK-8184765
>>>>> webrev: http://cr.openjdk.java.net/~gziemski/8184765_rev1
>>>>>
>>>>>
>>>>> cheers
>>>>>
>>>>
>>>
>