RFR (L) 7133093: Improve system dictionary performance
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri Jul 28 17:35:54 UTC 2017
Thank you for reviewing and your help and discussions.
On 7/28/17 11:51 AM, Karen Kinnear wrote:
> 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.
Oh, I see. Yes. In Dictionary::do_unloading() we could verify this.
Thanks,
Coleen
>
> 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
>>>>>>>
>>>>>>> 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