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 07:06:29 UTC 2018
Hi Lois,
----
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.
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?
---
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?
----
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.
----
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?
----
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?
----
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.
+ // 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".
----
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.
----
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. 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.
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.
----
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