RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent (original) (raw)
David Holmes david.holmes at oracle.com
Mon Oct 22 01:57:55 UTC 2018
- Previous message: RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent
- Next message: RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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::resolvewellknownclasses must not be replaced by JVMTI ClassFileLoadHook during run time, including - field offsets computed in JavaClasses::computeoffsets - the layout of the strings objects in the shared strings table The "well-known" classes can be replaced by ClassFileLoadHook only when JvmtiExport::earlyclasshookenv() 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 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
> 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
- Previous message: RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent
- Next message: RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]