RFR (L) 7133093: Improve system dictionary performance
Karen Kinnear
karen.kinnear at oracle.com
Fri Jul 28 15:51:14 UTC 2017
Coleen,
Ship it! webrev.03 looks good (I know you had other reviewers for the SA parts - I did not look at those).
Sorry to slow you down. Many thanks for the incremental diff - and thank you for the incremental changes.
Thank you for the eagerly created dictionaries - much simpler.
Thank you for all the experimentation and testing.
Thank you for the performance runs.
One potential future rfe:
>>
>> Future optimization:
>> I think that none of the built-in loaders is ever recorded as an initiating
>> loader for any class in a loader which permits unloading.
>> Note: we need to ensure that built-in includes boot, platform and
>> application class loader (and clean up any confusion with system
>> class loader - which can be set by the user to a custom class loader)
>>
> I think AppClassLoader can initiate loading and is a builtin loader.
>
> Dictionary for class loader class loader 0x00007f6abc6e3210 a 'jdk/internal/loader/ClassLoaders$AppClassLoader'{0x0000000101c8df00}
> Java system dictionary (table_size=107, classes=1)
> ^ indicates that initiating loader is different from defining loader
> 106: ^java.util.ArrayList, loader NULL class_loader
Let me clarify.
It is not that they can not initiate loading, it is that they are ever recorded as an
initiating loader for any other loaders other than our 3 built-in loaders which never unload.
So the potential future improvement would be no need to walk our 3 built-in loaders to check if we
have initiating loaders referencing dead defining loaders.
thanks,
Karen
>
> Thanks!
> Coleen
>
>>
>> thanks,
>> Karen
>>
>>
>>> On Jul 17, 2017, at 2:37 PM, coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com> wrote:
>>>
>>>
>>> Hi, I have made changes for Ioi's comments below. The largest change since .01 is that PredictedLoadedClassCount is used to size dictionaries for class loader data, and should be used to specify the average number of classes loaded by each class loader. Since this is an option available via UnlockExperimentalVMOptions, I'm not sure what the process is for changing the meaning of this option. We will work with our customers who have used this option for specifying the total number of classes loaded. We are also working on the change to allow automatic resizing of the dictionaries during safepoints.
>>>
>>> See:
>>> open webrev at http://cr.openjdk.java.net/~coleenp/7133093.02/webrev <http://cr.openjdk.java.net/%7Ecoleenp/7133093.02/webrev>
>>>
>>> Tested with the full nightly test set (jdk, nsk, hotspot jtreg tests) on linux x64.
>>>
>>> Thanks,
>>> Coleen
>>>
>>> On 6/30/17 4:45 PM, coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com> wrote:
>>>>
>>>> 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 <mailto: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 <http://cr.openjdk.java.net/%7Ecoleenp/7133093.01/webrev>
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-7133093 <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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list