<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <p>Hi Serguei,</p>
    <p>Thanks for the review!<br>
    </p>
    <br>
    <div class="moz-cite-prefix">On 10/22/18 5:09 PM,
      <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:84e95bbf-7e94-2554-a049-f67044dd7767@oracle.com">
      <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
      <div class="moz-cite-prefix">Hi Ioi,<br>
        <br>
        It looks good to me.<br>
        Some minor questions below.<br>
        <br>
        <br>
        <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><br>
        <pre><span class="changed"> 706   // Resolve well_known classes so they can be used like SystemDictionary::String_klass()</span></pre>
          Q1 (really minor): Did you want to spell well_known as
        well-known as in the javaClasses.cpp?<br>
        <br>
        <br>
      </div>
    </blockquote>
    Fixed<br>
    <br>
    <blockquote type="cite"
      cite="mid:84e95bbf-7e94-2554-a049-f67044dd7767@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><br>
        <pre><span class="new"> 420           assert(!SystemDictionary::is_well_known_klass(resolved_k),</span>
<span class="new"> 421                  "shared well-known classes must not be replaced by JVMTI ClassFileLoadHook");</span>
<span class="new"> 422           ResourceMark rm;</span>
<span class="new"> 423           log_info(cds, heap)("Failed to load subgraph because %s was not loaded from archive",</span>
<span class="new"> 424                               resolved_k->external_name());</span>
</pre>
         Q2: Would it make sense to move the assert after the log_info?
        <br>
        <br>
        <br>
      </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).<br>
    <br>
    <br>
    <blockquote type="cite"
      cite="mid:84e95bbf-7e94-2554-a049-f67044dd7767@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><br>
        <pre> 160                 if (checkSubgraph && false) {</pre>
         Q3: Could you explain false a little bit?<br>
        <br>
      </div>
    </blockquote>
    <br>
    Good catch. I left it there by mistake. I will remove the
    "&& false".<br>
    <br>
    Thanks<br>
    - Ioi<br>
    <br>
    <blockquote type="cite"
      cite="mid:84e95bbf-7e94-2554-a049-f67044dd7767@oracle.com">
      <div class="moz-cite-prefix"> Thanks,<br>
        Serguei<br>
        <br>
        <br>
        On 10/21/18 22:49, Ioi Lam wrote:<br>
      </div>
      <blockquote type="cite"
        cite="mid:ca2335dd-4978-1b10-60a8-b1ca6769834e@oracle.com">Hi
        David, <br>
        <br>
        Thanks for the review. Updated webrev: <br>
        <br>
        <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>
        <br>
        <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>
        <br>
        <br>
        More comments below: <br>
        <br>
        <br>
        <br>
        On 10/21/18 6:57 PM, David Holmes wrote: <br>
        <blockquote type="cite">Hi Ioi, <br>
          <br>
          Generally seems okay. <br>
          <br>
          On 22/10/2018 11:15 AM, Ioi Lam wrote: <br>
          <blockquote type="cite">Re-sending to the correct mailing
            lists. Please disregard the other email. <br>
            <br>
            <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>
            <br>
            <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>
            <br>
            <br>
            Hi, <br>
            <br>
            CDS has various built-in assumptions that classes loaded by
            <br>
            SystemDictionary::resolve_well_known_classes must not be
            replaced <br>
            by JVMTI ClassFileLoadHook during run time, including <br>
            <br>
            - field offsets computed in JavaClasses::compute_offsets <br>
            - the layout of the strings objects in the shared strings
            table <br>
            <br>
            The "well-known" classes can be replaced by
            ClassFileLoadHook only <br>
            when JvmtiExport::early_class_hook_env() is true. Therefore,
            the <br>
            fix is to disable CDS under this condition. <br>
          </blockquote>
          <br>
          I'm a little unclear why we have to iterate JvmtiEnv list when
          this has to be checked during JVMTI_PHASE_PRIMORDIAL? <br>
          <br>
        </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()"? <br>
        <br>
        <br>
        bool JvmtiExport::early_class_hook_env() { <br>
          JvmtiEnvIterator it; <br>
          for (JvmtiEnv* env = it.first(); env != NULL; env =
        it.next(env)) { <br>
            if (env->early_class_hook_env()) { <br>
              return true; <br>
            } <br>
          } <br>
          return false; <br>
        } <br>
        <br>
        This function matches condition in the existing code that would
        call into ClassFileLoadHook: <br>
        <br>
        class JvmtiClassFileLoadHookPoster { <br>
         ... <br>
          void post_all_envs() { <br>
            JvmtiEnvIterator it; <br>
            for (JvmtiEnv* env = it.first(); env != NULL; env =
        it.next(env)) { <br>
                .. <br>
                post_to_env(env, true); <br>
            } <br>
          } <br>
        ... <br>
          void post_to_env(JvmtiEnv* env, bool caching_needed) { <br>
            if (env->phase() == JVMTI_PHASE_PRIMORDIAL &&
        !env->early_class_hook_env()) { <br>
              return; <br>
            } <br>
        <br>
        <br>
        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. <br>
        <br>
        <br>
        <br>
        <blockquote type="cite">
          <blockquote type="cite">I have added a few test cases to try
            to replace shared classes, <br>
            including well-known classes and other classes. See <br>
            comments in ReplaceCriticalClasses.java for details. <br>
            <br>
            As a clean up, I also renamed all use of "preloaded" in <br>
            the source code to "well-known". They refer to the same
            thing <br>
            in HotSpot, so there's no need to use 2 terms. Also, The
            word <br>
            "preloaded" is ambiguous -- it's unclear when "preloading"
            happens, <br>
            and could be confused with what CDS does during archive dump
            time. <br>
          </blockquote>
          <br>
          A few specific comments: <br>
          <br>
          src/hotspot/share/classfile/systemDictionary.cpp <br>
          <br>
          + bool SystemDictionary::is_well_known_klass(Symbol*
          class_name) { <br>
          +   for (int i = 0; ; i++) { <br>
          +     int sid = wk_init_info[i]; <br>
          +     if (sid == 0) { <br>
          +       break; <br>
          +     } <br>
          <br>
          I take it a zero value is a guaranteed end-of-list sentinel? <br>
          <br>
        </blockquote>
        <br>
        Yes. The array is defined just a few lines above: <br>
        <br>
        static const short wk_init_info[] = { <br>
          #define WK_KLASS_INIT_INFO(name, symbol) \ <br>
            ((short)vmSymbols::VM_SYMBOL_ENUM_NAME(symbol)), <br>
        <br>
          WK_KLASSES_DO(WK_KLASS_INIT_INFO) <br>
          #undef WK_KLASS_INIT_INFO <br>
          0 <br>
        }; <br>
        <br>
        Also, <br>
        <br>
        class vmSymbols: AllStatic { <br>
          enum SID { <br>
            NO_SID = 0, <br>
            .... <br>
        <br>
        <br>
        <br>
        <blockquote type="cite">+ for (int i=FIRST_WKID; i<last; i++)
          { <br>
          <br>
          Style nit: need spaces around = and < <br>
          <br>
        </blockquote>
        <br>
        Fixed. <br>
        <br>
        <blockquote type="cite">--- <br>
          <br>
test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java
          <br>
          <br>
          New file should have current copyright year only. <br>
          <br>
        </blockquote>
        Fixed. <br>
        <br>
        <blockquote type="cite"> 31  * @comment CDS should not be
          disabled -- these critical classes will be replaced because
          JvmtiExport::early_class_hook_env() is true. <br>
          <br>
          Comment seems contradictory: if we replace critical classes
          then CDS should be disabled right?? <br>
          <br>
        </blockquote>
        Fixed. <br>
        <br>
        <blockquote type="cite">I expected to see tests that checked
          for: <br>
          <br>
          "CDS is disabled because early JVMTI ClassFileLoadHook is in
          use." <br>
          <br>
          in the output. ?? <br>
          <br>
        </blockquote>
        <rant> <br>
        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. <br>
        </rant> <br>
        <br>
        Now the test checks that whether CDS is indeed disabled, whether
        the affected class is loaded from the shared archive, etc. <br>
        <br>
        Thanks <br>
        - Ioi <br>
        <br>
        <blockquote type="cite">Thanks, <br>
          David <br>
          <br>
          <br>
          <blockquote type="cite"> <br>
             > In early e-mails Jiangli wrote: <br>
             > <br>
             > We should consider including more classes from the
            default classlist <br>
             > in the test. Archived classes loaded during both
            'early' phase and after <br>
             > should be tested. <br>
            <br>
            Done. <br>
            <br>
            <br>
             > For future optimizations, we might want to prevent
            loading additional <br>
             > shared classes if any of the archived system classes
            is changed. <br>
            <br>
            What's the benefit of doing this? Today we already stop
            loading a shared <br>
            class if its super class was not loaded from the archive. <br>
            <br>
            <br>
            Thanks <br>
            - Ioi <br>
            <br>
          </blockquote>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>