<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div>Hi Roman,</div><div><br>On 25 Oct 2017, at 17:54, Roman Kennke <<a href="mailto:rkennke@redhat.com">rkennke@redhat.com</a>> wrote:<br><br></div><blockquote type="cite"><div>
  
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  
  
    <div class="moz-cite-prefix">Hi Erik,<br>
      <br>
      thank you for your suggestions and changes.<br>
      <br>
      I like it: if you look at webrev.00 you will notice that my
      original proposal had both the static stuff and the virtual stuff
      in the same class too. So from there it's mostly the renaming
      exercise :-)<br></div></div></blockquote><div><br></div><div>Yeah. Sounds like we are aligned.</div><br><blockquote type="cite"><div><div class="moz-cite-prefix">
      So let me just put your changes up for review (again), if you
      don't mind:<br>
      <br>
      Full webrev:
      <br>
      <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/">http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
      </a><br>
      <br>
      Incremental over webrev.02:
      <br>
      <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.02_03/">
        http://cr.openjdk.java.net/~eosterlund/8189171/webrev.02_03/
      </a><br>
      <br>
      So I suppose we can count it by reviewed ok by you?<br></div></div></blockquote><div><br></div><div>Yes. Looks fantastic.</div><br><blockquote type="cite"><div><div class="moz-cite-prefix">
      Then it requires one more review to go in, right?<br></div></div></blockquote><div><br></div><div>Yup.</div><div><br></div><div>Thanks,</div><div>/Erik</div><br><blockquote type="cite"><div><div class="moz-cite-prefix">
      Thanks, Roman<br>
      <br>
      <br>
    </div>
    <blockquote type="cite" cite="mid:59F0AD40.3020307@oracle.com">Hi
      Roman,
      <br>
      <br>
      Definitely looks better. So the static GCArgumentProcessor has a
      singleton GCCollectedHeapFactory that is created during
      GCArgumentProcessor::initialize().
      <br>
      That makes sense to me, but I think it can be even better.
      <br>
      <br>
      First off - sorry for bikeshedding a bit here.
      <br>
      <br>
      To save you from having to update the code due to my bikeshedding,
      I have prepared a patch to show you what I meant, and you can tell
      me whether you agree or not.
      <br>
      <br>
      Full webrev:
      <br>
      <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/">http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/</a>
      <br>
      <br>
      Incremental over yours:
      <br>
      <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eosterlund/8189171/webrev.02_03/">http://cr.openjdk.java.net/~eosterlund/8189171/webrev.02_03/</a>
      <br>
      <br>
      My changes are the following:
      <br>
      <br>
      I collapsed the all-static GCArgumentProcessor and the
      CollectedHeapFactory into the same class called GCArguments (the
      natural GC helper for Arguments). It's a singleton object that
      deals with GC arguments stuff.
      <br>
      <br>
      GCArguments has some static functions to create_instance(),
      creating the singleton GCArguments object, and after you have
      created the singleton instance, you access it through
      GCArguments::instance(), and it will point to a GC-specific
      GCArguments, e.g. G1Arguments, CMSArguments, etc.
      <br>
      <br>
      The main reason why I think it's better to have a GC-specific
      GCArguments class compared to a CollectedHeapFactory class, is
      that I would like to eventually move all arguments processing into
      the GCArguments class, regardless of where (before or after
      CollectedHeap exists) in the boot strapping process. For example,
      today some argument stuff is dumped in the arguments.cpp file, and
      other stuff is dumped in CollectorPolicy::initialize_flags which
      is called later when the heap exists where it seemingly reads
      values out from flags, into internal variables, then changes them
      to what they should be and write the values back to the flags, and
      subsequently asserts that the internal and flag variants have the
      same value. Seems to me like we could in a subsequent refactoring
      move some of that kind of stuff into GCArguments instead, so that
      the argument setting logic is contained in the same class rather
      than split all over the place. But that seems like a separate RFE.
      <br>
      <br>
      Other than that, noticed some precompile header include was
      missing, the GCArgumentProcessor::initialize() function returned
      jint but it always returned JNI_OK, and called fatal() if
      something went wrong, instead of logging an error and returning
      JNI_ERR. And now that these types are collapsed, I moved
      initialize_flags_global into the abstract initial_flags, allowing
      GCs to override it completely if necessary, but in practice having
      them now call the super initial_flags(). So yeah, went ahead and
      fixed that for you.
      <br>
      <br>
      As for your subsequent patch creating the heap, that will just be
      a virtual call into the specific GCArguments class.
      <br>
      <br>
      I hope you agree with my proposal. It seemed easier for me to
      provide a patch describing what I'm thinking than to write it down
      in text.
      <br>
      <br>
      Thanks,
      <br>
      /Erik
      <br>
      <br>
      On 2017-10-23 17:19, Roman Kennke wrote:
      <br>
      <blockquote type="cite">Hi Erik,
        <br>
        <br>
        thanks for your suggestions. So I renamed 'GC' ->
        'CollectedHeapFactory' (and subclasses likewise) and 'GCFactory'
        to 'GCArgumentProcessor'. This will make even more sense once I
        added heap creation to CollectedHeapFactory (JDK-8189389).
        <br>
        <br>
        Differential:
        <br>
        <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~rkennke/8189171/webrev.02.diff/">http://cr.openjdk.java.net/~rkennke/8189171/webrev.02.diff/</a>
        <a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02.diff/"><http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02.diff/></a>
        <br>
        <br>
        Full webrev:
        <br>
        <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~rkennke/8189171/webrev.02/">http://cr.openjdk.java.net/~rkennke/8189171/webrev.02/</a>
        <a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02/"><http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02/></a>
        <br>
        <br>
        Better?
        <br>
        <br>
        Roman
        <br>
        <br>
        <blockquote type="cite">Hi Roman,
          <br>
          <br>
          I'm usually not one to really mind names too much, but I don't
          believe some bootstrapping code for the GC should hog the name
          "GC".
          <br>
          <br>
          Instead of GC::initialize(), I'm thinking a static
          GCArgumentProcessor::initialize() and instead of
          GC::factory(), I am thinking a static
          CollectedHeapFactory::create(). Or something like that.
          <br>
          <br>
          The reason is that I think the name GC is too general for the
          bootstraping-only problems its code touches. Hope I am making
          sense.
          <br>
          <br>
          Thanks,
          <br>
          /Erik
          <br>
          <br>
          On 2017-10-20 14:25, Roman Kennke wrote:
          <br>
          <blockquote type="cite">Hi Erik,
            <br>
            <br>
            Yes that is fine.
            <br>
            <br>
            Keep in mind that I'm planning to put heap creation into
            that class too.
            <br>
            <br>
            I was thinking maybe to simply reverse the current naming:
            name the all-static 'factory factory' 'GC', and thus call
            GC::initialize() and GC::factory() or such, and the main
            interface GCFactory or CollectedHeapFactory. What do you
            think?
            <br>
            <br>
            Roman
            <br>
            <br>
            <br>
            <blockquote type="cite">I would like to keep the
              CollectedHeap as the facade interface to the GC, like it
              is today, instead of having a new GC class making it two
              facade interfaces.
              <br>
              Of course, the CollectedHeap may only be used as a facade
              after it has been created. And now we are dealing with
              code before it was created during the bootstrapping
              process.
              <br>
              <br>
              So what I would like to have here is a class specifically
              for that, rather than another GC facade interface. I think
              this is mostly an exercise of finding a better name for
              the class you call "GC". Now I am not an expert in coming
              up with good names myself, but some name that indicates
              this is being used for flag processing would be good.
              Perhaps GCArgumentProcessor. The benefit of calling it
              something like that is that if a GC requires argument
              processing later in the bootstrapping, possibly after
              CollectedHeap has been created, it can still be used for
              this purpose without having to feel weird about it.
              <br>
              <br>
              How would you feel about that? Does it seem to make sense?
              <br>
              <br>
              Thanks,
              <br>
              /Erik
              <br>
              <br>
              On 2017-10-19 21:14, Roman Kennke wrote:
              <br>
              <blockquote type="cite">Am 13.10.2017 um 03:20 schrieb
                David Holmes:
                <br>
                <blockquote type="cite">Hi Roman,
                  <br>
                  <br>
                  Not a review as GC folk need to do that.
                  <br>
                  <br>
                  On 13/10/2017 5:59 AM, Roman Kennke wrote:
                  <br>
                  <blockquote type="cite">I'm posting it to both
                    hotspot-runtime-dev and hotspot-gc-dev because it
                    touches both areas (i.e. the GC interface).
                    <br>
                    <br>
                    Currently, all GC related argument processing is
                    done in arguments.cpp, littering it with #ifdef
                    INCLUDE_ALL_GCS and all sorts of GC specific methods
                    etc.
                    <br>
                  </blockquote>
                  <br>
                  From a runtime perspective I like all the GC specific
                  ifdefs and settings moved out-of-line of the main
                  argument processing.
                  <br>
                  <br>
                  From a refactoring perspective I noticed that
                  set_object_alignment() no longer calls
                  set_cms_values(). I presume setting it elsewhere is
                  okay?
                  <br>
                </blockquote>
                I totally forgot to reply to this.
                <br>
                <br>
                What's important is that the CMS alignment values are
                set after set_object_alignment() figured out the object
                alignment. The patch moves that a little further down
                the road to the beginning of its GC specific argument
                processing, but from GC perspective should be the same.
                I looked thoroughly through the involved code paths and
                cannot see what could go wrong.
                <br>
                <br>
                I need one more review. GC folks? The current webrev is:
                <br>
                <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/">http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/</a>
                <a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/"><http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/></a>
                <br>
                <br>
                Thanks,
                <br>
                Roman
                <br>
                <br>
              </blockquote>
              <br>
            </blockquote>
            <br>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <p><br>
    </p>
  

</div></blockquote></body></html>