RFR (L) 7133093: Improve system dictionary performance

[coleen.phillimore at oracle.com]

Ioi,  Thank you for looking at this change.

On 6/30/17 2:39 PM, Ioi Lam wrote:
> Hi Coleen,
>
> Maybe the bug should be renamed to "Use one Dictionary per class 
> loader instance"? That way it's more obvious what it is when you look 
> at the repo history.

I can do that.  I made it One Dictionary per ClassLoaderData
>
> 1.
>
> Why is assert_locked_or_safepoint(SystemDictionary_lock) necessary in 
> SystemDictionary::find_class (line 1826), but not necessary 
> SystemDictionary::find (line 951)? Since you removed 
> NoSafepointVerifier nosafepoint in the latter, maybe this means it's 
> safe to remove the assert_locked_or_safepoint in the former?

The call to SystemDictionary::find() is the (I believe) usual lock free 
lookup and the SystemDictionary::find_class() is used to verify that a 
class we're about to add or want to add hasn't been added by another 
thread.   Or certain cases where we already have a lock to do something 
else, like add loader constraints.  I took out the NoSafepointVerifier 
because it assumes that system dictionary entries would be moved by GC, 
which they won't.  The old hash function could safepoint when getting 
the hash for the class_loader object.

>
> 2.
>
>  455 static ClassLoaderData* _current_loader_data = NULL;
>  456 static Klass*           _current_class_entry = NULL;
>  457
>  458 InstanceKlass* ClassLoaderDataGraph::try_get_next_class() {
>
> How about moving the static fields into an iterator object. That way 
> you don't need to keep track of the globals
>
> ClassLoaderDataGraphIterator {
>    ClassLoaderData* _current_loader_data
>    Klass*           _current_class_entry;
>
>    InstanceKlass* try_get_next_class() { ....}
> };

Ok, there's a different iterator that iterates over all of the classes 
for GC.  I will adapt that for this use.   That would be better.

>
> 3. Double check locking in ClassLoaderData::dictionary() -- someone 
> else should look at this :-)

I copied code that David Holmes added for modules.

>
> 4. We may need a better strategy for deciding the size of each 
> dictionary.
>
>  565 const int  _primelist[10] = {1, 107, 1009};
>  571 Dictionary* ClassLoaderData::dictionary() {
>  579     if ((dictionary = _dictionary) == NULL) {
>  580       int size;
>  581       if (this == the_null_class_loader_data() || 
> is_system_class_loader_data()) {
>  582         size = _primelist[2];
>  583       } else if 
> (class_loader()->is_a(SystemDictionary::reflect_DelegatingClassLoader_klass())) 
> {
>  584         size = _primelist[0];
>  585       } else {
>  586         size = _primelist[1];
>  587       }
>  588       dictionary = new Dictionary(this, size);
>
> I'll do some investigation on this issue and get back to you.
>

How about if someone uses PredictedLoadedClassCount, then we use that to 
size all but the reflection and boot class loader?  Then if there's an 
application that has a class loader with a huge amount of classes loaded 
in it, that would help this?  It might cost some footprint but an 
oversized table would simply be a bigger array of pointers, so it might 
not be that bad to oversize.

I think the long term solution is being able to resize these entries and 
make the application provide arguments.   Please let me know what you 
find in your investigation and if that would work.
> The rest of the changes look good to me.

Thank you!
Coleen
>
> Thanks
> - Ioi
>
>
> On 6/23/17 4:42 PM, coleen.phillimore at oracle.com wrote:
>> Summary: Implement one dictionary per ClassLoaderData for faster 
>> lookup and removal during class unloading
>>
>> See RFE for more details.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/7133093.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-7133093
>>
>> Tested with full "nightly" run in rbt, plus locally class loading and 
>> unloading tests:
>>
>> jtreg hotspot/test/runtime/ClassUnload
>>
>> jtreg hotspot/test/runtime/modules
>>
>> jtreg hotspot/test/gc/class_unloading
>>
>> make test-hotspot-closed-tonga FILTER=quick TEST_JOBS=4 
>> TEST=vm.parallel_class_loading
>>
>> csh ~/testing/run_jck9  (vm/lang/java_lang)
>>
>> runThese -jck - uses class loader isolation to run each jck test and 
>> unloads tests when done (at -gc:5 intervals)
>>
>>
>> Thanks,
>> Coleen
>>
>>
>