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'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.
- 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.
-// 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?
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
