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
Fri Jun 15 23:52:50 UTC 2018
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