On 10/22/18 10:56 AM, Ioi Lam wrote:
On 10/22/18 10:25 AM, Jiangli Zhou wrote:
Hi Ioi,
Looks good. Please see comments below.
- src/hotspot/share/classfile/javaClasses.cpp
4254 assert(JvmtiEnvBase::get_phase() <= JVMTI_PHASE_PRIMORDIAL,
4255 "Field offsets of well-known classes must be computed
in JVMTI_PHASE_PRIMORDIAL or before");
Maybe it is worth adding a function (for example, is_primordial()) in
jvmtiEnvBase.hpp, so we can avoid using JVMTI details here?
I'll add JvmtiExport::is_early_phase(), since the phase can be either
JVMTI_PHASE_ONLOAD or JVMTI_PHASE_PRIMORDIAL.
I'm not too sure if the assert is necessary. Why well known-classes'
field offsets must be computed in JVMTI_PHASE_PRIMORDIAL or before?
Currently they are, however that's because they are used early by the
VM. That doesn't directly relate to any JVMTI phase necessarily. With
the assert, we are explicitly making a connection to the JVMTI
phases. To me, that doesn't seem to be necessary.
JavaClasses::compute_offsets uses many different classes. I would need
to check that each of them were in the well-known class list, so that
we know the offsets stored in the CDS archive are still valid.
However, I couldn't find a single place to make such a check, and it
would be much easier to add the above assert, which means any shared
class used inside compute_offsets cannot be replaced by JVMTI.
How about this:
void JavaClasses::compute_offsets() {
if (UseSharedSpaces) {
assert(JvmtiEnvBase::is_early_phase() &&
!JvmtiExport::has_early_class_hook_env(),
"JavaClasses::compute_offsets() must be called in early
JVMTI phase.");
// None of the classes used by the rest of this function can be
replaced by
// JMVTI ClassFileLoadHook.
// We are safe to use the archived offsets, which have already
been restored
// by JavaClasses::serialize_offsets, without computing the
offsets again.
return;
}
You could do assert(k->is_shared() || !UseSharedSpaces) in the
DO_SERIALIZE_OFFSETS macro to make sure the expected shared classes are
used when UseSharedSpaces is enabled during loading the archived field
offsets. The extra !UseSharedSpaces here is because serialize_offsets()
function is used for both dump time and runtime. It could be removed
because the 'is_shared' flag is probably already set when we writing out
data at dump time, but please double check that.
#define DO_SERIALIZE_OFFSETS(k) k::serialize_offsets(soc);
If JvmtiExport::early_class_hook_env is requested by a JVMTI agent,
UseSharedSpaces should already be disabled at runtime, otherwise a
shared class should be used in this case.
- src/hotspot/share/classfile/systemDictionary.cpp
2108 if (UseSharedSpaces) {
2109 assert(JvmtiEnvBase::get_phase() <= JVMTI_PHASE_PRIMORDIAL,
2110 "All well known classes must be resolved in
JVMTI_PHASE_PRIMORDIAL or before");
2111 for (int i = FIRST_WKID; i < last; i++) {
2112 InstanceKlass* k = _well_known_klasses[i];
2113 assert(k->is_shared(), "must not be replaced by JVMTI
class file load hook");
2114 }
Please include the above block under #ifdef ASSERT.
OK
-// preloading is actually performed by resolve_preloaded_classes.
+// class resolution is actually performed by
resolve_well_known_classes.
The original comment is more accurate. Maybe use 'eager loading' if
you want to avoid the confusion between 'preloading' and CDS's term?
I can see that "class resolution" could have different meanings,
although resolve_well_known_classes does call
SystemDictionary::resolve_or_fail, not any the SystemDictionary::load*
functions. So using the word "resolve" would be more appropriate.
How about changing the comments to the following to avoid ambiguity.
#define WK_KLASS_ENUM_NAME(kname) kname##_knum
// Certain classes, such as java.lang.Object and java.lang.String,
// are "well-known", in the sense that no class loader is allowed
// to provide a different definition.
//
// Each well-known class has a short klass name (like object_klass),
// and a vmSymbol name (like java_lang_Object).
//
// The order of these definitions is significant: the classes are
// resolved during early VM start-up by resolve_well_known_classes
// in this order. Changing the order may require careful restructuring
// of the VM start-up sequence.
//
#define WK_KLASSES_DO(do_klass) ......
Looks ok.
Thanks,
Jiangli
Thanks
- Ioi
The test looks good. Thanks for filling the gap in this area!
Thanks,
Jiangli
On 10/21/18 10:49 PM, Ioi Lam wrote:
Hi David,
Thanks for the review. Updated webrev:
http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/
http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04.delta/
More comments below:
On 10/21/18 6:57 PM, David Holmes wrote:
Hi Ioi,
Generally seems okay.
On 22/10/2018 11:15 AM, Ioi Lam wrote:
Re-sending to the correct mailing lists. Please disregard the
other email.
http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v03/
https://bugs.openjdk.java.net/browse/JDK-8212200
Hi,
CDS has various built-in assumptions that classes loaded by
SystemDictionary::resolve_well_known_classes must not be replaced
by JVMTI ClassFileLoadHook during run time, including
- field offsets computed in JavaClasses::compute_offsets
- the layout of the strings objects in the shared strings table
The "well-known" classes can be replaced by ClassFileLoadHook only
when JvmtiExport::early_class_hook_env() is true. Therefore, the
fix is to disable CDS under this condition.
I'm a little unclear why we have to iterate JvmtiEnv list when this
has to be checked during JVMTI_PHASE_PRIMORDIAL?
I think you are asking about this new function? I don't like the
name "early_class_hook_env()". Maybe I should change it to
"has_early_class_hook_env()"?
bool JvmtiExport::early_class_hook_env() {
JvmtiEnvIterator it;
for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
if (env->early_class_hook_env()) {
return true;
}
}
return false;
}
This function matches condition in the existing code that would call
into ClassFileLoadHook:
class JvmtiClassFileLoadHookPoster {
...
void post_all_envs() {
JvmtiEnvIterator it;
for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
..
post_to_env(env, true);
}
}
...
void post_to_env(JvmtiEnv* env, bool caching_needed) {
if (env->phase() == JVMTI_PHASE_PRIMORDIAL &&
!env->early_class_hook_env()) {
return;
}
post_all_envs() is called just before a class is about to be loaded
in the JVM. So if *any* env->early_class_hook_env() returns true,
there's a chance that it will replace a well-known class.So, as a
preventive measure, CDS will be disabled.
I have added a few test cases to try to replace shared classes,
including well-known classes and other classes. See
comments in ReplaceCriticalClasses.java for details.
As a clean up, I also renamed all use of "preloaded" in
the source code to "well-known". They refer to the same thing
in HotSpot, so there's no need to use 2 terms. Also, The word
"preloaded" is ambiguous -- it's unclear when "preloading" happens,
and could be confused with what CDS does during archive dump time.
A few specific comments:
src/hotspot/share/classfile/systemDictionary.cpp
+ bool SystemDictionary::is_well_known_klass(Symbol* class_name) {
+ for (int i = 0; ; i++) {
+ int sid = wk_init_info[i];
+ if (sid == 0) {
+ break;
+ }
I take it a zero value is a guaranteed end-of-list sentinel?
Yes. The array is defined just a few lines above:
static const short wk_init_info[] = {
#define WK_KLASS_INIT_INFO(name, symbol) \
((short)vmSymbols::VM_SYMBOL_ENUM_NAME(symbol)),
WK_KLASSES_DO(WK_KLASS_INIT_INFO)
#undef WK_KLASS_INIT_INFO
0
};
Also,
class vmSymbols: AllStatic {
enum SID {
NO_SID = 0,
....
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <
Fixed.
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.
Fixed.
31 * @comment CDS should not be disabled -- these critical
classes will be replaced because
JvmtiExport::early_class_hook_env() is true.
Comment seems contradictory: if we replace critical classes then
CDS should be disabled right??
Fixed.
I expected to see tests that checked for:
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??
<rant>
It would have been easy if jtreg lets you check the output of @run
easily. Instead, your innocent suggestion has turned into 150+ lines
of new code :-( Maybe "let's write all shell tests in Java" isn't
such a great idea after all.
</rant>
Now the test checks that whether CDS is indeed disabled, whether the
affected class is loaded from the shared archive, etc.
Thanks
- Ioi
Thanks,
David
> In early e-mails Jiangli wrote:
>
> We should consider including more classes from the default
classlist
> in the test. Archived classes loaded during both 'early' phase
and after
> should be tested.
Done.
> For future optimizations, we might want to prevent loading
additional
> shared classes if any of the archived system classes is changed.
What's the benefit of doing this? Today we already stop loading a
shared
class if its super class was not loaded from the archive.
Thanks
- Ioi
