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 18:26:37 UTC 2018
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.
>
> 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.
>
> ---
>
> 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 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.
>
>
> 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. I too was actually thinking of "classname @<unitialized>"
so I do like that approach but it is a rare case.
>
> ----
>
> 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.
>
> ----
>
> 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.
>
> + // 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.
>
> ----
>
> 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? 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.
>
> ----
>
> 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.
> 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?
>
> 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?
Thanks,
Lois
>
> ----
>
> 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