Okay, thanks!
Serguei
On 10/22/18 18:49, Ioi Lam wrote:
Hi Serguei,
Thanks for the review!
Fixed
Actually, the assert checks for a condition that should never
happen, and the log_info can happen in normal execution (when the
class being replaced is not a well-known klass).
Good catch. I left it there by mistake. I will remove the
"&& false".
Thanks
- Ioi
Thanks,
Serguei
On 10/21/18 22:49, 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
|