RFR (M) JDK-8202605: Standardize on ClassLoaderData::loader_name() throughout the VM to obtain a class loader's name
Thomas Stüfe
thomas.stuefe at gmail.com
Fri Jun 15 19:43:41 UTC 2018
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