RFR (M) JDK-8202605: Standardize on ClassLoaderData::loader_name() throughout the VM to obtain a class loader's name
Lois Foltan
lois.foltan at oracle.com
Tue Jun 19 11:29:49 UTC 2018
On 6/18/2018 3:42 PM, coleen.phillimore at oracle.com wrote:
>
> This version is good as well. Thank you for working through all the
> concerns.
> Coleen
Thank you Coleen!
Lois
>
> On 6/15/18 7:52 PM, Lois Foltan wrote:
>> Hi Thomas,
>>
>> I have read through all your comments below, thank you. I think the
>> best compromise that hopefully will enable this change to go forward
>> is to back out my changes to classfile/classLoaderHierarchyDCmd.cpp
>> and classfile/classLoaderStats.cpp. This will allow the
>> serviceability team to review the new format for the class loader's
>> name_and_id and go forward if applicable to jcmd in a follow on RFE.
>> Updated webrev at:
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202605.3/webrev/
>>
>> Thanks,
>> Lois
>>
>> On 6/15/2018 3:43 PM, Thomas Stüfe wrote:
>>
>>> Hi Lois,
>>>
>>> On Fri, Jun 15, 2018 at 8:26 PM, Lois Foltan
>>> <lois.foltan at oracle.com> wrote:
>>>> On 6/15/2018 3:06 AM, Thomas Stüfe wrote:
>>>>
>>>> Hi Lois,
>>>>
>>>> Hi Thomas,
>>>> Thank you for looking at this change and giving it another round of
>>>> review!
>>>>
>>>> ----
>>>>
>>>> We have now:
>>>>
>>>> Symbol* ClassLoaderData::name()
>>>>
>>>> which returns ClassLoader.name
>>>>
>>>> and
>>>>
>>>> const char* ClassLoaderData::loader_name()
>>>>
>>>> which returns either ClassLoader.name or, if that is null, the
>>>> class
>>>> name.
>>>>
>>>> I would like to point out that ClassLoaderData::loader_name() is
>>>> pretty much
>>>> unchanged as it exists today, so this behavior is not new or changed.
>>>>
>>> Okay.
>>>
>>>> 1) if we keep it that way, we should at least rename loader_name() to
>>>> something like loader_name_or_class_name() to lessen the surprise.
>>>>
>>>> 2) But maybe these two functions should have the same behaviour?
>>>> Return name or null if not set, not the class name? I see that nobody
>>>> yet uses loader_name(), so you are free to define it as you see fit.
>>>>
>>>> 3) but if (2), maybe alternativly just get rid of loader_name()
>>>> altogether, as just calling as_C_string() on a symbol is not worth a
>>>> utility function?
>>>>
>>>> I would like to leave ClassLoaderData::loader_name() in for a
>>>> couple of
>>>> reasons. Leaving it in discourages new methods like it to be
>>>> introduced in
>>>> the future in data structures other than ClassLoaderData, calling
>>>> java_lang_ClassLoader::name() directly is not safe during unloading
>>>> and
>>>> getting rid of it may force a call to as_C_string() as option #3
>>>> suggests
>>>> but that doesn't handle the bootstrap class loader. Given this I
>>>> think the
>>>> best course of action would be to update ClassLoaderData.hpp with
>>>> the same
>>>> comments I put in place within ClassLoaderData.cpp for this method
>>>> as you
>>>> suggest below.
>>> Okay.
>>>
>>>>
>>>> ---
>>>>
>>>> For VM.systemdictionary, the texts seem to be a bit off:
>>>>
>>>> 29167:
>>>> Dictionary for loader data: 0x00007f7550cb8660 for instance a
>>>> 'jdk/internal/reflect/DelegatingClassLoader'{0x0000000706c00000}
>>>>
>>>> "for instance a" ?
>>>>
>>>> Dictionary for loader data: 0x00007f75503b3a50 for instance a
>>>> 'jdk/internal/loader/ClassLoaders$AppClassLoader'{0x000000070647b098}
>>>> Dictionary for loader data: 0x00007f75503a4e30 for instance a
>>>> 'jdk/internal/loader/ClassLoaders$PlatformClassLoader'{0x0000000706479088}
>>>>
>>>>
>>>> should that not be "app" or "platform", respectively?
>>>>
>>>> ... but I just see it was the same way before and not touched by your
>>>> change. Maybe here, your new compound name would make sense?
>>>>
>>>> ----
>>>>
>>>> If I understand correctly this output shows up when one specifies
>>>> -Xlog:class+load=debug?
>>> I saw it as result of jcmd VM.systemdictionary (Coleen's command, I
>>> think?) but it may show up too in other places, I did not check.
>>>
>>>> I see that the "for instance " is printed by
>>>>
>>>> void ClassLoaderData::print_value_on(outputStream* out) const {
>>>> if (!is_unloading() && class_loader() != NULL) {
>>>> out->print("loader data: " INTPTR_FORMAT " for instance ",
>>>> p2i(this));
>>>> class_loader()->print_value_on(out); // includes
>>>> loader_name_and_id()
>>>> and address of class loader instance
>>>>
>>>> and class_loader()->print_value_on(out); eventually calls
>>>> InstanceKlass::oop_print_value_on to print the "a".
>>>>
>>>> void InstanceKlass::oop_print_value_on(oop obj, outputStream* st) {
>>>> st->print("a ");
>>>> name()->print_value_on(st);
>>>> obj->print_address_on(st);
>>>> if (this == SystemDictionary::String_klass()
>>>>
>>>> This is a good follow up RFE since one will have to look at all the
>>>> calls to
>>>> InstanceKlass::oop_print_value_on() to determine if the "a " is still
>>>> applicable.
>>>>
>>> Yes, there may be a number of follow up cleanups after this patch is
>>> in.
>>>
>>>>
>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202605.1/webrev/src/hotspot/share/classfile/classLoaderData.cpp.sdiff.html
>>>>
>>>>
>>>> Good comments.
>>>>
>>>> suggested change to comment:
>>>>
>>>> 129 // Obtain the class loader's name and identity hash. If the
>>>> class loader's
>>>> 130 // name was not explicitly set during construction, the class
>>>> loader's name and id
>>>> 131 // will be set to the qualified class name of the class loader
>>>> along with its
>>>> 132 // identity hash.
>>>>
>>>> rather:
>>>>
>>>> 129 // Obtain the class loader's name and identity hash. If the
>>>> class loader's
>>>> 130 // name was not explicitly set during construction, the class
>>>> loader's ** _name_and_id field **
>>>> 131 // will be set to the qualified class name of the class loader
>>>> along with its
>>>> 132 // identity hash.
>>>>
>>>> Done.
>>>>
>>>>
>>>> ----
>>>>
>>>> 133 // If for some reason the ClassLoader's constructor has not
>>>> been run, instead of
>>>>
>>>> I am curious, how can this happen? Bad bytecode instrumentation?
>>>> Should we also attempt to work in the identity hashcode in that case
>>>> to be consistent with the java side? Or maybe name it something like
>>>> "classname <uninitialized>"? Or is this too exotic a case to care?
>>>>
>>>> Bad bytecode instrumentation, Unsafe.allocateInstance(), see test
>>>> open/test/hotspot/jtreg/runtime/modules/ClassLoaderNoUnnamedModuleTest.java
>>>>
>>>> for example.
>>> JDK-8202758... Wow. Yikes.
>>>
>>>> I too was actually thinking of "classname @<unitialized>" so I
>>>> do like that approach but it is a rare case.
>>>>
>>> Thanks for taking that suggestion.
>>>
>>>> ----
>>>>
>>>> In various places I see you using:
>>>>
>>>> 937 if (_class_loader_klass == NULL) { // bootstrap case
>>>>
>>>> just to make sure, this is the same as
>>>> CLD::is_the_null_class_loader_data(), yes? So, one could use one and
>>>> assert the other?
>>>>
>>>> Yes. Actually Coleen & I were discussing that maybe we could remove
>>>> ClassLoaderData::_class_loader_klass since its original purpose was
>>>> to allow
>>>> for ultimately a way to obtain the class loader's klass
>>>> external_name. Will
>>>> look into creating a follow on RFE if _class_loader_klass is no longer
>>>> needed.
>>>>
>>> I use it in VM.classloaders and VM.metaspace, to print out the loader
>>> class name and in VM.classloaders verbose mode I print out the Klass*
>>> pointer too. We found it useful in some debugging scenarios.
>>>
>>> Btw, for the same reason I print out the "{loader oop}" in
>>> VM.classloaders - debugging help. This was also a wish of Kirk
>>> Pepperdine when we introduced VM.classloaders, see discussion:
>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-May/023770.html
>>>
>>> .
>>>
>>> There are discussions and experiments currently done to execute
>>> multiple jcmd subcommands at one safe point. In this context, printing
>>> oops is more interesting in diagnostic commands, since you can chain
>>> multiple commands together and get consistent oop values. See
>>> discussions here:
>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-May/023673.html
>>>
>>> (Currently, Frederic Parain from Oracle took this over and provided a
>>> prototype patch).
>>>
>>> But all in all, if it makes matters easier, I think yes we should
>>> remove _class_loader_klass from CLD.
>>>
>>>> ----
>>>>
>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202605.1/webrev/src/hotspot/share/classfile/classLoaderData.hpp.sdiff.html
>>>>
>>>>
>>>> Not sure about BOOTSTRAP_LOADER_NAME_LEN, since its sole user - jfr -
>>>> could probably just do a ::strlen(BOOTSTRAP_LOADER_NAME).
>>>>
>>>> Not sure either about BOOTSTRAP_LOADER_NAME having quotes baked in -
>>>> this is something I would rather see in the printing code.
>>>>
>>>> I agree. I removed the single quotes but I would like to leave in
>>>> BOOTSTAP_LOADER_NAME_LEN.
>>>>
>>> Okay. We should make sure they stay consistent, but that is no
>>> terrible burden.
>>>
>>>> + // Obtain the class loader's _name, works during unloading.
>>>> + const char* loader_name() const;
>>>> + Symbol* name() const { return _name; }
>>>>
>>>> See above my comments to loader_name(). At the very least comment
>>>> should be updated describing that this function returns name or class
>>>> name or "bootstrap".
>>>>
>>>> Comment in ClassLoaderData.hpp will be updated as you suggest.
>>>>
>>> Thank you.
>>>
>>>>
>>>> ----
>>>>
>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202605.1/webrev/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.udiff.html
>>>>
>>>>
>>>> Hm, unfortunately, this does not look so good. I would prefer to keep
>>>> the old version, see here my proposal, updated to use your new
>>>> CLD::name() function and to remove the offending "<>" around
>>>> "bootstrap".
>>>>
>>>> @@ -157,13 +157,18 @@
>>>>
>>>> // Retrieve information.
>>>> const Klass* const loader_klass = _cld->class_loader_klass();
>>>> + const Symbol* const loader_name = _cld->name();
>>>>
>>>> branchtracker.print(st);
>>>>
>>>> // e.g. "+--- jdk.internal.reflect.DelegatingClassLoader"
>>>> st->print("+%.*s", BranchTracker::twig_len, "----------");
>>>> - st->print(" %s,", _cld->loader_name_and_id());
>>>> - if (!_cld->is_the_null_class_loader_data()) {
>>>> + if (_cld->is_the_null_class_loader_data()) {
>>>> + st->print(" bootstrap");
>>>> + } else {
>>>> + if (loader_name != NULL) {
>>>> + st->print(" \"%s\",", loader_name->as_C_string());
>>>> + }
>>>> st->print(" %s", loader_klass != NULL ?
>>>> loader_klass->external_name() : "??");
>>>> st->print(" {" PTR_FORMAT "}", p2i(_loader_oop));
>>>> }
>>>>
>>>> This also depends on what you decide happens with CLD::loader_name().
>>>> If that one were to return "loader name or null if not set, as
>>>> ra-allocated const char*", it could be used here.
>>>>
>>>> I like this change and I like how the output looks. Can you take
>>>> another
>>>> look at the next webrev's updated comments in test
>>>> serviceability/dcmd/vm/ClassLoaderHierarchyTest.java?
>>> Sure. It is not yet posted, yes?
>>>
>>> May take till monday though, I am gone over the weekend.
>>>
>>>> I plan to open an RFE
>>>> to have the serviceability team consider removing the address of
>>>> the loader
>>>> oop now that the included identity hash provides unique
>>>> identification.
>>>>
>>> See my remarks above - that command including oop was added by me, and
>>> if possible I'd like to keep the oop for debugging purposes. However,
>>> I could move the output to the "verbose" section (if you run
>>> VM.classloaders verbose, there are additional things printed below the
>>> class loader name).
>>>
>>> Note however, that printing "{<oop>}" was consistent with pre-existing
>>> commands from Oracle, in this case VM.systemdictionary.
>>>
>>>>
>>>> ----
>>>>
>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202605.1/webrev/src/hotspot/share/classfile/classLoaderStats.cpp.udiff.html
>>>>
>>>>
>>>> In VM.classloader_stats we see the effect of the new naming:
>>>>
>>>> x000000080000a0b8 0x00000008000623f0 0x00007f5facafe540 1
>>>> 6144 4064 jdk.internal.reflect.DelegatingClassLoader @7b5a12ae
>>>> 0x000000080000a0b8 0x00000008000623f0 0x00007f5facbcdd50 1
>>>> 6144 3960 jdk.internal.reflect.DelegatingClassLoader @5b529706
>>>> 0x00000008000623f0 0x0000000000000000 0x00007f5facbcca00 10
>>>> 90112 51760 'MyInMemoryClassLoader' @17cdf2d0
>>>> 0x00000008000623f0 0x0000000000000000 0x00007f5facbca560 1
>>>> 6144 4184 'MyInMemoryClassLoader' @1477089c
>>>> 0x00000008000623f0 0x0000000000000000 0x00007f5facba7890 1
>>>> 6144 4184 'MyInMemoryClassLoader' @a87f8ec
>>>> 0x00000008000623f0 0x0000000000000000 0x00007f5facba5390 1
>>>> 6144 4184 'MyInMemoryClassLoader' @5a3bc7ed
>>>> 0x00000008000623f0 0x0000000000000000 0x00007f5facba3bf0 1
>>>> 6144 4184 'MyInMemoryClassLoader' @48c76607
>>>> 0x00000008000623f0 0x0000000000000000 0x00007f5facb23f80 1
>>>> 6144 4184 'MyInMemoryClassLoader' @1224144a
>>>> 0x00000008000623f0 0x0000000000000000 0x00007f5facb228f0 1
>>>> 6144 4184 'MyInMemoryClassLoader' @75437611
>>>> 0x00000008000623f0 0x0000000000000000 0x00007f5facb65c60 1
>>>> 6144 4184 'MyInMemoryClassLoader' @25084a1e
>>>> 0x00000008000623f0 0x0000000000000000 0x00007f5facb6a030 1
>>>> 6144 4184 'MyInMemoryClassLoader' @2d2ffcb7
>>>> 0x00000008000623f0 0x0000000000000000 0x00007f5facb4bfe0 1
>>>> 6144 4184 'MyInMemoryClassLoader' @42a48628
>>>> 0x0000000800010340 0x00000008000107a8 0x00007f5fac3bd670 1064
>>>> 7004160 6979376 'app'
>>>> 96
>>>> 311296 202600 + unsafe anonymous classes
>>>> 0x0000000000000000 0x0000000000000000 0x00007f5fac1da1e0 1091
>>>> 8380416 8301048 'bootstrap'
>>>> 92
>>>> 263168 169808 + unsafe anonymous classes
>>>> 0x000000080000a0b8 0x000000080000a0b8 0x00007f5faca63460 1
>>>> 6144 3960 jdk.internal.reflect.DelegatingClassLoader @5bd03f44
>>>>
>>>>
>>>>> Since we hide now the class name of the loader, if everyone names
>>>>> their class loader the same - e.g. "Test" or
>>>>> "MyInMemoryClassLoader" -
>>>>> we loose information.
>>>> We loose the name of class loader's class' fully qualified name
>>>> only in the
>>>> situation where the class loader's name has been explicitly
>>>> specified by the
>>>> user during construction. I would think in that case one would
>>>> want to see
>>>> the explicitly given name of the class loader. We also gain in either
>>>> situation (unnamed or named class loader), the class loader's
>>>> identity hash
>>>> which allows for uniquely identifying a class loader in question.
>>> For the record, I would prefer a naming scheme which printed
>>> unconditionally both name and class name, if both are set:
>>>
>>> '"name", instance of <class>, @id'
>>>
>>> or
>>>
>>> 'instance of <class>, @id'
>>>
>>> or maybe some more condensed, technical form, as a clear triple:
>>>
>>> '[name, <class>, @id]' or '{name, <class>, @id}'
>>>
>>> The reason why I keep harping on this is that it is useful to have
>>> consistent output, meaning, output that does not change its format on
>>> a line-by-line base.
>>>
>>> Just a tiny example why this is useful, lets say I run a Spring MVC
>>> app and want to know the number of Spring loaders, I do a:
>>>
>>> ./images/jdk/bin/jcmd hello VM.classloaders | grep
>>> org.springframework | wc -l
>>>
>>> Won't work consistently anymore if class names disappear for loader
>>> names which have names.
>>>
>>> Of course, there are myriad other ways to get the same information, so
>>> this is just an illustration.
>>>
>>> --
>>>
>>> But I guess I won't convince you that this is better, and it seems you
>>> spent a lot of thoughts and discussions on this point already. I think
>>> this is a case of one-size-fits-not-all. And also a matter of taste.
>>>
>>> If emphasis is on brevity, your naming scheme is better. If
>>> ease-of-parsing and ease-of-reading are important, I think my scheme
>>> wins.
>>>
>>> But as long as we have alternatives - e.g. CLD::name() and
>>> CLD::class_loader_class() - and as long as VM.classloaders and
>>> VM.metaspace commands stay useful, I am content and can live with your
>>> scheme.
>>>
>>>>> I'm afraid this will be an issue if people will
>>>>> start naming their class loaders more and more. It is not
>>>>> unimaginable
>>>>> that completely different frameworks name their loaders the same.
>>>> Point taken, however, doesn't including the identity hash allow for
>>>> unique
>>>> identification of the class loader?
>>> I think the point of diagnostic commands is to get information quick.
>>> An identity hash may help me after I managed to finally resolve it,
>>> but it is not a quick process (that I know of). Whereas, for example,
>>> just reading "com.wily.introscope.Loader" tells me immediately that
>>> the VM I am looking at has Wily byte code modifications enabled.
>>>
>>>>
>>>>> This "name or if not then class name" scheme will also complicate
>>>>> parsing a lot for people who parse the output of these commands. I
>>>>> would strongly prefer to see both - name and class type.
>>>> Much like classfile/classLoaderHierarchyDCmd.cpp now generates,
>>>> correct?
>>>>
>>> Yes! :)
>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>>
>>> I just saw your webrev popping in, but again it is late. I'll take a
>>> look tomorrow morning or monday. Thank you for your work.
>>>
>>> ..Thomas
>>>
>>>> ----
>>>>
>>>> Hmm. At this point I noticed that I still had general reservations
>>>> about the new compound naming scheme - see my remarks above. So I
>>>> guess I stop here to wait for your response before continuing the code
>>>> review.
>>>>
>>>> Thanks & Kind Regards,
>>>>
>>>> Thomas
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Thu, Jun 14, 2018 at 9:56 PM, Lois Foltan
>>>> <lois.foltan at oracle.com> wrote:
>>>>
>>>> Please review this updated webrev that address review comments
>>>> received.
>>>>
>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202605.1/webrev/
>>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>>
>>>> On 6/13/2018 6:58 PM, Lois Foltan wrote:
>>>>
>>>> Please review this change to standardize on how to obtain a class
>>>> loader's
>>>> name within the VM. SystemDictionary::loader_name() methods have been
>>>> removed in favor of ClassLoaderData::loader_name().
>>>>
>>>> Since the loader name is largely used in the VM for display purposes
>>>> (error messages, logging, jcmd, JFR) this change also adopts a new
>>>> format to
>>>> append to a class loader's name its identityHashCode and if the
>>>> loader has
>>>> not been explicitly named it's qualified class name is used instead.
>>>>
>>>> 391 /**
>>>> 392 * If the defining loader has a name explicitly set then
>>>> 393 * '<loader-name>' @<id>
>>>> 394 * If the defining loader has no name then
>>>> 395 * <qualified-class-name> @<id>
>>>> 396 * If it's built-in loader then omit `@<id>` as there is only one
>>>> instance.
>>>> 397 */
>>>>
>>>> The names for the builtin loaders are 'bootstrap', 'app' and
>>>> 'platform'.
>>>>
>>>> open webrev at
>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202605/webrev/
>>>> bug link at https://bugs.openjdk.java.net/browse/JDK-8202605
>>>>
>>>> Testing: hs-tier(1-2), jdk-tier(1-2) complete
>>>> hs-tier(3-5), jdk-tier(3) in progress
>>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>>
>>
>
More information about the hotspot-dev
mailing list