RFR (M) JDK-8202605: Standardize on ClassLoaderData::loader_name() throughout the VM to obtain a class loader's name
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Jun 18 19:42:00 UTC 2018
This version is good as well. Thank you for working through all the
concerns.
Coleen
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