RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent

Hi Ioi,

Generally seems okay.

I'm a little unclear why we have to iterate JvmtiEnv list when this has
to be checked during JVMTI_PHASE_PRIMORDIAL?

Post by Ioi Lam
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?

+ for (int i=FIRST_WKID; i<last; i++) {

Style nit: need spaces around = and <

---

test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java

New file should have current copyright year only.

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??

I expected to see tests that checked for:

"CDS is disabled because early JVMTI ClassFileLoadHook is in use."

in the output. ??

Thanks,
David

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

Ioi Lam

2018-10-22 05:49:03 UTC

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/

Post by David Holmes
Hi Ioi,
Generally seems okay.

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.

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,
    ....

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
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.

Post by David Holmes
"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

Post by David Holmes
Thanks,
David

Post by Ioi Lam
>
> 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

Jiangli Zhou

2018-10-22 17:25:21 UTC

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

Post by Ioi Lam
Hi David,
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/

Post by David Holmes
Hi Ioi,
Generally seems okay.

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
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.

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,
    ....

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
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.

Post by David Holmes
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??

<rant>
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

Post by David Holmes
Thanks,
David

Post by Ioi Lam
>
> 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

Ioi Lam

2018-10-22 17:56:49 UTC

Post by David Holmes
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.

Post by David Holmes
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;
}

Post by David Holmes
- 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.

Post by David Holmes
-// 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) ......

Thanks
- Ioi

Post by David Holmes
The test looks good. Thanks for filling the gap in this area!
Thanks,
Jiangli

Post by David Holmes
Hi Ioi,
Generally seems okay.

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
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.

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
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.

Post by David Holmes
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??

<rant>
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

Post by David Holmes
Thanks,
David

Post by Ioi Lam
>
> 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

Jiangli Zhou

2018-10-22 22:06:06 UTC

I'll add JvmtiExport::is_early_phase(), since the phase can be either
JVMTI_PHASE_ONLOAD or JVMTI_PHASE_PRIMORDIAL.

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.
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.

Post by David Holmes
- 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.

Post by David Holmes
-// 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

Post by Ioi Lam
Thanks
- Ioi

Post by David Holmes
The test looks good. Thanks for filling the gap in this area!
Thanks,
Jiangli

Post by David Holmes
Hi Ioi,
Generally seems okay.

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
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.

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
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.

Post by David Holmes
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??

<rant>
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

Post by David Holmes
Thanks,
David

Post by Ioi Lam
>
> 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

Ioi Lam

2018-10-23 01:43:22 UTC

Post by Jiangli Zhou

Post by David Holmes
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.

Post by David Holmes
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.
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.

Hi Jiangli,

Thanks for the suggestion.

I tried this, but The "k" parameter to this macro is not an
InstanceKlass. Instead, it's these guys

#define BASIC_JAVA_CLASSES_DO_PART2(f) \
f(java_lang_System) \
f(java_lang_ClassLoader) \
f(java_lang_Throwable) \
f(java_lang_Thread) \
f(java_lang_ThreadGroup) \
f(java_lang_AssertionStatusDirectives) \
f(java_lang_ref_SoftReference) \
f(java_lang_invoke_MethodHandle) \
f(java_lang_invoke_DirectMethodHandle) \
f(java_lang_invoke_MemberName) \
f(java_lang_invoke_ResolvedMethodName) \
f(java_lang_invoke_LambdaForm) \
f(java_lang_invoke_MethodType) \
f(java_lang_invoke_CallSite) \
f(java_lang_invoke_MethodHandleNatives_CallSiteContext) \
f(java_security_AccessControlContext) \
f(java_lang_reflect_AccessibleObject) \
f(java_lang_reflect_Method) \
f(java_lang_reflect_Constructor) \
f(java_lang_reflect_Field) \
f(java_nio_Buffer) \
f(reflect_ConstantPool) \
f(reflect_UnsafeStaticFieldAccessorImpl) \
f(java_lang_reflect_Parameter) \
f(java_lang_Module) \
f(java_lang_StackTraceElement) \
f(java_lang_StackFrameInfo) \
f(java_lang_LiveStackFrameInfo) \
f(java_util_concurrent_locks_AbstractOwnableSynchronizer) \
//end

I can somehow force this to work by reworking all macros related to
BASIC_JAVA_CLASSES_DO, but that doesn't seem to be worth it. I think my
current assert is stronger than necessary so it's safe and simple.

Thanks
- Ioi

Post by Jiangli Zhou

Post by David Holmes
- 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.

Post by David Holmes
-// 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

Post by Ioi Lam
Thanks
- Ioi

Post by David Holmes
The test looks good. Thanks for filling the gap in this area!
Thanks,
Jiangli

Post by David Holmes
Hi Ioi,
Generally seems okay.

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
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.

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
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.

Post by David Holmes
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??

<rant>
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

Post by David Holmes
Thanks,
David

Post by Ioi Lam
>
> 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

Jiangli Zhou

2018-10-23 01:46:46 UTC

Sounds good. Thanks for trying that.

Thanks,
Jiangli

Post by David Holmes
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.

Post by David Holmes
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.
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.

Hi Jiangli,
Thanks for the suggestion.
I tried this, but The "k" parameter to this macro is not an InstanceKlass. Instead, it's these guys
#define BASIC_JAVA_CLASSES_DO_PART2(f) \
f(java_lang_System) \
f(java_lang_ClassLoader) \
f(java_lang_Throwable) \
f(java_lang_Thread) \
f(java_lang_ThreadGroup) \
f(java_lang_AssertionStatusDirectives) \
f(java_lang_ref_SoftReference) \
f(java_lang_invoke_MethodHandle) \
f(java_lang_invoke_DirectMethodHandle) \
f(java_lang_invoke_MemberName) \
f(java_lang_invoke_ResolvedMethodName) \
f(java_lang_invoke_LambdaForm) \
f(java_lang_invoke_MethodType) \
f(java_lang_invoke_CallSite) \
f(java_lang_invoke_MethodHandleNatives_CallSiteContext) \
f(java_security_AccessControlContext) \
f(java_lang_reflect_AccessibleObject) \
f(java_lang_reflect_Method) \
f(java_lang_reflect_Constructor) \
f(java_lang_reflect_Field) \
f(java_nio_Buffer) \
f(reflect_ConstantPool) \
f(reflect_UnsafeStaticFieldAccessorImpl) \
f(java_lang_reflect_Parameter) \
f(java_lang_Module) \
f(java_lang_StackTraceElement) \
f(java_lang_StackFrameInfo) \
f(java_lang_LiveStackFrameInfo) \
f(java_util_concurrent_locks_AbstractOwnableSynchronizer) \
//end
I can somehow force this to work by reworking all macros related to BASIC_JAVA_CLASSES_DO, but that doesn't seem to be worth it. I think my current assert is stronger than necessary so it's safe and simple.
Thanks
- Ioi

Post by David Holmes
- 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.

Post by David Holmes
-// 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

Post by David Holmes
The test looks good. Thanks for filling the gap in this area!
Thanks,
Jiangli

Post by David Holmes
Hi Ioi,
Generally seems okay.

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;
}
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.

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?

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,
....

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
Comment seems contradictory: if we replace critical classes then CDS should be disabled right??

Fixed.

Post by David Holmes
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??

<rant>
</rant>
Now the test checks that whether CDS is indeed disabled, whether the affected class is loaded from the shared archive, etc.
Thanks
- Ioi

Post by David Holmes
Thanks,
David

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

Lois Foltan

2018-10-22 17:25:53 UTC

Hi Ioi,

Looks good. A couple of comments:

classfile/systemDictionary.cpp
- line#1975 - My preference is to declare the variable sid outside the
for statement and compare sid == 0 within the for loop conditional.
- line#1981 - can you use class_name->fast_compare(symbol) for the
equality check?

memory/heapShared.cpp
- line#422 could be a ResourceMark rm(THREAD);

Thanks,
Lois

Post by David Holmes
Hi Ioi,
Generally seems okay.

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
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.

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
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.

Post by David Holmes
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??

Post by David Holmes
Thanks,
David

Post by Ioi Lam
>
> 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

Ioi Lam

2018-10-23 04:09:41 UTC

Hi Ioi,
classfile/systemDictionary.cpp
- line#1975 - My preference is to declare the variable sid outside the
for statement and compare sid == 0 within the for loop conditional.

Do you mean this?

bool SystemDictionary::is_well_known_klass(Symbol* class_name) {
int sid;
for (int i = 0; (sid = wk_init_info[i]) != 0; i++) {
    Symbol* symbol = vmSymbols::symbol_at((vmSymbols::SID)sid);
    if (class_name == symbol) {
      return true;
    }
}
return false;
}

Post by David Holmes
- line#1981 - can you use class_name->fast_compare(symbol) for the
equality check?

The comments around fast_compare says it's for vtable sorting only.

Post by David Holmes
memory/heapShared.cpp
- line#422 could be a ResourceMark rm(THREAD);

Fixed.

Thanks
- Ioi

Post by David Holmes
Thanks,
Lois

Post by David Holmes
Hi Ioi,
Generally seems okay.

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
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.

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
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.

Post by David Holmes
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??

<rant>
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

Post by David Holmes
Thanks,
David

Post by Ioi Lam
>
> 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

Lois Foltan

2018-10-23 14:34:14 UTC

Hi Ioi,
classfile/systemDictionary.cpp
- line#1975 - My preference is to declare the variable sid outside
the for statement and compare sid == 0 within the for loop conditional.

Do you mean this?
bool SystemDictionary::is_well_known_klass(Symbol* class_name) {
int sid;
for (int i = 0; (sid = wk_init_info[i]) != 0; i++) {
    Symbol* symbol = vmSymbols::symbol_at((vmSymbols::SID)sid);
    if (class_name == symbol) {
      return true;
    }
}
return false;
}

Yes, I think that's better.

Post by David Holmes
- line#1981 - can you use class_name->fast_compare(symbol) for the
equality check?

The comments around fast_compare says it's for vtable sorting only.

Right, David pointed that out to me as well. Comment retracted.

Post by David Holmes
memory/heapShared.cpp
- line#422 could be a ResourceMark rm(THREAD);

Fixed.

Great, thanks!
Lois

Post by Ioi Lam
Thanks
- Ioi

Post by David Holmes
Thanks,
Lois

Post by David Holmes
Hi Ioi,
Generally seems okay.

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
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.

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
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.

Post by David Holmes
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??

<rant>
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

Post by David Holmes
Thanks,
David

Post by Ioi Lam
>
> 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

s***@oracle.com

2018-10-23 00:09:04 UTC

<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Ioi, 
 
It looks good to me. 
Some minor questions below. 
 
 
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/src/hotspot/share/classfile/systemDictionary.hpp.frames.html">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/src/hotspot/share/classfile/systemDictionary.hpp.frames.html</a> 
<pre> 706 // Resolve well_known classes so they can be used like SystemDictionary::String_klass()</pre>
Q1 (really minor): Did you want to spell well_known as
well-known as in the javaClasses.cpp? <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/src/hotspot/share/memory/heapShared.cpp.frames.html">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/src/hotspot/share/memory/heapShared.cpp.frames.html</a> <pre> 420 assert(!SystemDictionary::is_well_known_klass(resolved_k), 421 "shared well-known classes must not be replaced by JVMTI ClassFileLoadHook"); 422 ResourceMark rm; 423 log_info(cds, heap)("Failed to load subgraph because %s was not loaded from archive", 424 resolved_k->external_name());
</pre>
Q2: Would it make sense to move the assert after the log_info? 
 
 
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java.html">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java.html</a> 
<pre> 160 if (checkSubgraph && false) {</pre>
Q3: Could you explain false a little bit? 
 
Thanks, 
Serguei 
 
 
On 10/21/18 22:49, Ioi Lam wrote: 
</div>
<blockquote type="cite"
cite="mid:ca2335dd-4978-1b10-60a8-***@oracle.com">Hi
David,
 
 
Thanks for the review. Updated webrev:
 
 
<a class="moz-txt-link-freetext" href="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/</a>
 
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04.delta/">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04.delta/</a>
 
 
More comments below:
 
 
 
 
On 10/21/18 6:57 PM, David Holmes wrote:
 
<blockquote type="cite">Hi Ioi,
 
 
Generally seems okay.
 
 
On 22/10/2018 11:15 AM, Ioi Lam wrote:
 
<blockquote type="cite">Re-sending to the correct mailing lists.
Please disregard the other email.
 
 
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v03/">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v03/</a>
 
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8212200">https://bugs.openjdk.java.net/browse/JDK-8212200</a>
 
 
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.
 
</blockquote>
 
I'm a little unclear why we have to iterate JvmtiEnv list when
this has to be checked during JVMTI_PHASE_PRIMORDIAL?
 
 
</blockquote>
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.
 
 
 
 
<blockquote type="cite">
<blockquote type="cite">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.
 
</blockquote>
 
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?
 
 
</blockquote>
 
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,
 
 ....
 
 
 
 
<blockquote type="cite">+ for (int i=FIRST_WKID; i<last; i++) {
 
 
Style nit: need spaces around = and <
 
 
</blockquote>
 
Fixed.
 
 
<blockquote type="cite">---
 
 
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
 
 
New file should have current copyright year only.
 
 
</blockquote>
Fixed.
 
 
<blockquote type="cite"> 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??
 
 
</blockquote>
Fixed.
 
 
<blockquote type="cite">I expected to see tests that checked for:
 
 
"CDS is disabled because early JVMTI ClassFileLoadHook is in
use."
 
 
in the output. ??
 
 
</blockquote>
<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
 
 
<blockquote type="cite">Thanks,
 
David
 
 
 
<blockquote type="cite">
 
> 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
 
 
</blockquote>
</blockquote>
 
</blockquote>
 
</body>
</html>

Ioi Lam

2018-10-23 01:49:30 UTC

Hi Serguei,

Thanks for the review!

Post by David Holmes
Hi Ioi,
It looks good to me.
Some minor questions below.
http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/src/hotspot/share/classfile/systemDictionary.hpp.frames.html
706 // Resolve well_known classes so they can be used like
SystemDictionary::String_klass()
Â Q1 (really minor): Did you want to spell well_known as well-known as
in the javaClasses.cpp?

Fixed

Post by David Holmes
http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/src/hotspot/share/memory/heapShared.cpp.frames.html
420 assert(!SystemDictionary::is_well_known_klass(resolved_k),
421 "shared well-known classes must not be replaced by JVMTI
ClassFileLoadHook");
422 ResourceMark rm;
423 log_info(cds, heap)("Failed to load subgraph because %s was not
loaded from archive",
424 resolved_k->external_name());
Â Q2: Would it make sense to move the assert after the log_info?

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).

Post by David Holmes
http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java.html
160 if (checkSubgraph && false) {
Â Q3: Could you explain false a little bit?

Good catch. I left it there by mistake. I will remove the "&& false".

Thanks
- Ioi

Post by David Holmes
Thanks,
Serguei

Post by David Holmes
Hi Ioi,
Generally seems okay.

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
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.

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?

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,
Â Â Â ....

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
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.

Post by David Holmes
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??

<rant>
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

Post by David Holmes
Thanks,
David

Post by Ioi Lam
Â >
Â > 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

s***@oracle.com

2018-10-23 01:53:25 UTC

<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Okay, thanks! 
Serguei 
 
 
On 10/22/18 18:49, Ioi Lam wrote: 
</div>
<blockquote type="cite"
cite="mid:f6cbbc52-7afd-705d-1d1f-***@oracle.com">
Hi Serguei,
Thanks for the review! 

 
<div class="moz-cite-prefix">On 10/22/18 5:09 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> wrote: 
</div>
<blockquote type="cite"
cite="mid:84e95bbf-7e94-2554-a049-***@oracle.com">
<div class="moz-cite-prefix">Hi Ioi, 
 
It looks good to me. 
Some minor questions below. 
 
 
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/src/hotspot/share/classfile/systemDictionary.hpp.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/src/hotspot/share/classfile/systemDictionary.hpp.frames.html</a> 
<pre> 706 // Resolve well_known classes so they can be used like SystemDictionary::String_klass()</pre>
Q1 (really minor): Did you want to spell well_known as
well-known as in the javaClasses.cpp? 
 
 
</div>
</blockquote>
Fixed 
 
<blockquote type="cite"
cite="mid:84e95bbf-7e94-2554-a049-***@oracle.com">
<div class="moz-cite-prefix"> <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/src/hotspot/share/memory/heapShared.cpp.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/src/hotspot/share/memory/heapShared.cpp.frames.html</a> <pre> 420 assert(!SystemDictionary::is_well_known_klass(resolved_k), 421 "shared well-known classes must not be replaced by JVMTI ClassFileLoadHook"); 422 ResourceMark rm; 423 log_info(cds, heap)("Failed to load subgraph because %s was not loaded from archive", 424 resolved_k->external_name());
</pre>
Q2: Would it make sense to move the assert after the
log_info? 
 
 
</div>
</blockquote>
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). 
 
 
<blockquote type="cite"
cite="mid:84e95bbf-7e94-2554-a049-***@oracle.com">
<div class="moz-cite-prefix"> <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java.html</a> 
<pre> 160 if (checkSubgraph && false) {</pre>
Q3: Could you explain false a little bit? 
 
</div>
</blockquote>
 
Good catch. I left it there by mistake. I will remove the
"&& false". 
 
Thanks 
- Ioi 
 
<blockquote type="cite"
cite="mid:84e95bbf-7e94-2554-a049-***@oracle.com">
<div class="moz-cite-prefix"> Thanks, 
Serguei 
 
 
On 10/21/18 22:49, Ioi Lam wrote: 
</div>
<blockquote type="cite"
cite="mid:ca2335dd-4978-1b10-60a8-***@oracle.com">Hi
David, 
 
Thanks for the review. Updated webrev: 
 
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/</a>
 
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04.delta/"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04.delta/</a>
 
 
More comments below: 
 
 
 
On 10/21/18 6:57 PM, David Holmes wrote: 
<blockquote type="cite">Hi Ioi, 
 
Generally seems okay. 
 
On 22/10/2018 11:15 AM, Ioi Lam wrote: 
<blockquote type="cite">Re-sending to the correct mailing
lists. Please disregard the other email. 
 
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v03/"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v03/</a>
 
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8212200"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212200</a>
 
 
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. 
</blockquote>
 
I'm a little unclear why we have to iterate JvmtiEnv list
when this has to be checked during JVMTI_PHASE_PRIMORDIAL? 
 
</blockquote>
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. 
 
 
 
<blockquote type="cite">
<blockquote type="cite">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. 
</blockquote>
 
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?
 
 
</blockquote>
 
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, 
 .... 
 
 
 
<blockquote type="cite">+ for (int i=FIRST_WKID; i<last;
i++) { 
 
Style nit: need spaces around = and < 
 
</blockquote>
 
Fixed. 
 
<blockquote type="cite">--- 
 
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
 
 
New file should have current copyright year only. 
 
</blockquote>
Fixed. 
 
<blockquote type="cite"> 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?? 
 
</blockquote>
Fixed. 
 
<blockquote type="cite">I expected to see tests that checked
for: 
 
"CDS is disabled because early JVMTI ClassFileLoadHook is in
use." 
 
in the output. ?? 
 
</blockquote>
<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 
 
<blockquote type="cite">Thanks, 
David 
 
 
<blockquote type="cite"> 
> 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 
 
</blockquote>
</blockquote>
 
</blockquote>
 
</blockquote>
 
</blockquote>
 
</body>
</html>

David Holmes

2018-10-23 02:11:19 UTC

Hi Ioi,

Sorry for all the extra work in the test. ;-)

The updates look good to me (and I've seen follow up comments).

Thanks,
David

Post by David Holmes
Hi Ioi,
Generally seems okay.

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
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.

Post by David Holmes
+ for (int i=FIRST_WKID; i<last; i++) {
Style nit: need spaces around = and <

Fixed.

Post by David Holmes
---
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
New file should have current copyright year only.

Fixed.

Post by David Holmes
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.

Post by David Holmes
"CDS is disabled because early JVMTI ClassFileLoadHook is in use."
in the output. ??

<rant>
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

Post by David Holmes
Thanks,
David

Jiangli Zhou

2018-10-22 17:57:34 UTC