<html>
  <head>
    <meta content="text/html; charset=windows-1252"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi Erik,<br>
    <br>
    <div class="moz-cite-prefix">On 2014-10-08 13:20, Erik Österlund
      wrote:<br>
    </div>
    <blockquote cite="mid:2F82EFA3-F76D-4FDF-832F-8F85F3BB42D0@lnu.se"
      type="cite">
      <meta http-equiv="Content-Type" content="text/html;
        charset=windows-1252">
      <div style="word-wrap:break-word">
        <div>Greetings,</div>
        <div><br>
        </div>
        <div>So here in this email is attached a patch-file containing
          my changes since I don't have access to the cr-server.</div>
        If anyone could help me make a RFR with a bug-ID with this
        changeset, that would be great.
      </div>
    </blockquote>
    <br>
    Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.00/">http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.00/</a><br>
    bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8059936">https://bugs.openjdk.java.net/browse/JDK-8059936</a><br>
    <br>
    I ran this through JPRT and got this failure on Solaris x86:<br>
    <pre>"hotspot/src/share/vm/memory/specialized_oop_closures.hpp", line 222: Warning: Identifier expected instead of "}".
1 Warning(s) detected.
gmake[8]: *** [abstractCompiler.o] Error 2
gmake[7]: *** [the_vm] Error 2
gmake[6]: *** [fastdebug] Error 2
gmake[5]: *** [generic_build2] Error 2
gmake[4]: *** [fastdebug] Error 2

StefanK
</pre>
    <br>
    <blockquote cite="mid:2F82EFA3-F76D-4FDF-832F-8F85F3BB42D0@lnu.se"
      type="cite">
      <div style="word-wrap:break-word">
        <div><br>
          <div>== Implementation Summary ==</div>
          <div><br>
          </div>
          <div>=== Dispatching To Closures ===</div>
          <div>All dispatching to OopClosure go through an all static
            class OopClosureDispatcher. It uses SFINAE to find the most
            appropriate member function to call by checking certain
            conditions of the OopClosureType. For any template
            parameterized OopClosureType calls to member function X in
            {do_oop, do_metadata, do_klass, do_class_loader_data} it
            will attempt the following in this order: 1) Check if the
            OopClosureType is manually unspecialized for whatever reason
            (currently only used by the abstract classes OopClosure and
            ExtendedOopClosure), in that case use virtual call. 2) Check
            if OopClosureType (and not a parent) declares X_nv, then use
            a non-virtual call to it. Otherwise 3) check if
            OopClosureType (and not a parent) declares X, then use a
            non-virtual call to X (hence removing need to define both X
            and X_nv any more, but still being backward compatible).
            Otherwise 3) use a normal virtual call to X.</div>
          <div><br>
          </div>
          <div>The reason why checking that candidates for non-virtual
            calls are declared in the concrete OopClosureType and not a
            parent is to be safe rather than sorry in detecting
            "abstract" base classes that are never used by themselves.
            However their derived types are used, but the type passed in
            to oop_iterate is not the derived closure type but the
            abstract one. So in summary we make sure we make non-virtual
            calls to the derived closure types and not the base types.</div>
          <div><br>
          </div>
          <div>=== Dispatching To Klass ===</div>
          <div>Two mechanisms are used for retaining the OopClosureType
            when dispatching to it using a specific Klass.</div>
          <div>Attempt one hopes there is a separation of concern
            between finding oops and dispatching to OopClosureType. A
            virtual call is made to the Klass to either a) return back
            the oop maps or b) identify which Klass it is with a
            "DispatchTag".</div>
          <div>If oop maps are returned (InstanceKlass, OopArrayKlass,
            TypeArrayKlass) then they are iterated over from oop_iterate
            where all the type info about the closure is still known. If
            this is not supported (InstanceRefKlass,
            InstanceMirrorKlass, InstanceClassLoaderKlass), then the
            dispatch tag is used to call an inline template method
            defined in brand new Instance*Klass.inline.hpp files.</div>
          <div><br>
          </div>
          <div>The new methods in Instance*Klass.inline.hpp are
            (non-virtual) template variants of what the old
            oop_oop_iterate macros did. They use SFINAE to check if the
            OopClosureType expects metadata related calls too (if it's
            an ExtendedOopClosure) otherwise does nothing. Note that
            oop_iterate now takes an arbitrary type instead of
            ExtendedOopClosure, but it uses SFINAE to force template
            instantiations only if it's a subtype of ExtendedOopClosure
            and otherwise generate compiler errors and similarly on
            oop_iterate_no_header, specialization is used and it
            explicitly checks the template parameter is of type
            OopClosure. If it's an ExtendedOopClosure, it sends the
            first ever composite closure type
            NoHeaderOopClosure<OopClosureType> to the dispatch
            system so that metadata-related calls are never called in
            the first place. :)</div>
          <div><br>
          </div>
          <div>Thanks,</div>
          <div><br>
          </div>
          <div>/Erik</div>
        </div>
      </div>
      <div style="word-wrap:break-word">
        <div><br>
          <div>
            <div>On 01 Oct 2014, at 17:02, Erik Österlund <<a
                moz-do-not-send="true"
                href="mailto:erik.osterlund@lnu.se">erik.osterlund@lnu.se</a>>
              wrote:</div>
            <br class="x_Apple-interchange-newline">
            <blockquote type="cite">
              <div style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                <br class="x_Apple-interchange-newline">
                On 01 Oct 2014, at 16:11, Stefan Karlsson <<a
                  moz-do-not-send="true"
                  href="mailto:stefan.karlsson@oracle.com">stefan.karlsson@oracle.com</a>>
                wrote:</div>
              <br class="x_Apple-interchange-newline"
                style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
              <blockquote type="cite" style="font-family:Helvetica;
                font-size:12px; font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                <div style="font-size:12px; font-style:normal;
                  font-variant:normal; font-weight:normal;
                  letter-spacing:normal; line-height:normal;
                  orphans:auto; text-align:start; text-indent:0px;
                  text-transform:none; white-space:normal; widows:auto;
                  word-spacing:0px">
                  <br>
                  On 2014-10-01 15:41, Erik Österlund wrote:<br>
                  <blockquote type="cite">Hey,<br>
                    <br>
                    I have to admit I'm not a huge fan of the current
                    system for explicitly specializing closures
                    (removing virtual calls).<br>
                    <br>
                    Here's a couple of problems I see with the current
                    solution:<br>
                    1. The specialization macros are obviously not
                    pretty<br>
                    2. It's awkward to have to remember to explicitly
                    list the closure as specialized in the macros<br>
                    3. There are plenty of composite closure types out
                    there. What I mean by this is when closures are
                    combined, e.g. one closure is used to filter a
                    memory range, and if passing the filter, it will
                    invoke the actual closure, currently resulting in a
                    virtual call even though the composite structure is
                    completely known at the call site.<br>
                    4. Each closure has to have like do_oop, do_oop_v,
                    do_oop_nv, for both oop types and then a do_oop_work
                    for joining them. Yuck! Asserts try to check that
                    the _v and _nv methods do the same thing to combat
                    programmer mistakes.<br>
                    <br>
                    With my alternative template magic solution:<br>
                    1. You won't have to explicitly specialize wanted
                    closure types - they are automatically specialized
                    unless the contrary is explicitly stated.<br>
                    2. Parameterized composite closure types can be used
                    without unnecessary virtual call overheads.<br>
                    3. Only a single do_oop (do_metadata etc) member
                    function is needed, and hence no need to put asserts
                    trying to keep _v and _nv synchronized.<br>
                    4. It is backward compatible and does not require
                    major refactoring; could transition into this system
                    step by step. The two systems can even co-exist.<br>
                    5. It supports an interface where OopClosure is the
                    interface to oop_iterate, rather than
                    ExtendedOopClosure. It uses SFINAE to send metadata
                    info to the closure only if the derived type is an
                    ExtendedOopClosure, otherwise it simply sends the
                    oops (do_oop) only. (of course I can remove this if
                    it's unwanted and we intentionally don't want to
                    support oop_iterate(OopClosure*) )<br>
                    <br>
                    For the interested reader, this is how the old
                    system worked:<br>
                    The ugly macros generate overloads of oop_iterate on
                    oopDesc which uses a virtual call to the Klass (also
                    using macro generated overloads) to figure out where
                    the oops are and then call the closure. This step
                    with the virtual call to the Klass to call the
                    closure removes any potential for template magic
                    because template member functions can't be virtual
                    in C++.<br>
                    <br>
                    And this is how my system solves this:<br>
                    A template oop_iterate (with closure type as
                    parameter) member function uses a virtual call to
                    the Klass, but only to acquire information where
                    oops can be found (and NOT to call the actual
                    closure too). It then uses static template
                    polymorphism (CRTP idiom) to invoke the do_oop
                    method of the corresponding derived closure types
                    (without virtual calls). This required support from
                    the Klass implementations. I currently support
                    object arrays and normal instances. If the Klass
                    implementation does not support this new scheme, it
                    simply reverts to a normal virtual call like before.<br>
                    As a bonus I made a new include file in
                    utilities/templateIdioms.hpp with some template
                    magic I needed and which I was missing but could
                    likely be used in more places in the future.<br>
                    <br>
                    Would this change be interesting for the GC group?
                    In that case I could prepare a patch (and perhaps
                    add support to the other Klass implementations). :)<br>
                    I would also need some help to check if this works
                    on your wide range of platforms and compilers etc
                    (only checked the assembly output for my own setup).<br>
                  </blockquote>
                  <br>
                  Yes, please provide a patch!<br>
                  <br>
                  I've had a patch to start solving some of these
                  problems for a long time:<br>
                  <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Estefank/prototype/oop_iterate_dispatch/webrev.00/">http://cr.openjdk.java.net/~stefank/prototype/oop_iterate_dispatch/webrev.00/</a><br>
                  <br>
                  one problem that I haven't solved in my patch is how
                  to pass down the OopClosureType past the
                  Klass::oop_iterate virtual calls. See oop.inline.hpp:<br>
                  template <bool nv, typename OopClosureType><br>
                  inline int oopDesc::oop_iterate(OopClosureType* blk) {<br>
                   CONCRETE_KLASS_DO_AND_RETURN(klass(),
                  oop_oop_iterate<nv>(this,blk));<br>
                  }<br>
                  <br>
                  and klass.inline.hpp for the dispatch code:<br>
                  <br>
                  +#define CONCRETE_KLASS_DO_AND_RETURN(the_klass, todo)
                  \<br>
                  +  do { \<br>
                  +    switch ((the_klass)->dispatch_tag()) { \<br>
                  +    case Klass::_instance:
                                          return
                  InstanceKlass::cast(the_klass)->todo; \<br>
                  +    case Klass::_instance_ref:
                                   return
                  InstanceRefKlass::cast(the_klass)->todo; \<br>
                  +    case Klass::_instance_mirror:            return
                  InstanceMirrorKlass::cast(the_klass)->todo; \<br>
                  +    case Klass::_instance_class_loader: return
                  InstanceClassLoaderKlass::cast(the_klass)->todo; \<br>
                  +    case Klass::_obj_array:
                                         return
                  ObjArrayKlass::cast(the_klass)->todo; \<br>
                  +    case Klass::_type_array:
                                       return
                  TypeArrayKlass::cast(the_klass)->todo; \<br>
                  +    default: fatal("Incorrect dispatch index");
                  return 0; \<br>
                  +    }   \<br>
                  +  } while (false)<br>
                  +<br>
                </div>
              </blockquote>
              <div style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                <br>
              </div>
              <div style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                Yeah, I was faced with the same problem. It's impossible
                in C++ to pass template arguments to virtual member
                functions. (Note however that it's possible to have
                template parameterized /types/ with virtual member
                functions, so with your choice of using enums with a
                dispatch tag I think it should be possible to have a
                KlassDispatchProxy<ClosureType> cl;
                cl.get_and_dispatch(obj); where KlassDispatchProxy type
                (rather than Klass type) is picked using the enum and
                get_and_dispatch is a virtual method for the
                parameterized type).</div>
              <div style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                <br>
              </div>
              <div style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                However, my choice was to instead recognize finding oop
                maps and dispatching to the closure as two fundamentally
                separate concerns. So instead I implemented a new
                virtual Klass member function to only get the oop maps
                (if supported) of an oop (for InstanceKlass simply
                return the array of oop map blocks, for ObjArrayKlass
                simply return an oop map dummy with the interval of oops
                for the array). This way, I can get the oop maps with a
                virtual call without having to know the ClosureType
                needed for dispatching, and then when I have the oop
                maps, simply dispatc (partially using SFINAE to dispatch
                the stuff the closure expects to receive; depending on
                if it's an ExtendedOopClosure or not).</div>
              <div style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                <br>
              </div>
              <div style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                I'm glad this is something useful. Currently I only
                enable this for InstanceKlass and ObjArrayKlass,
                otherwise it reverts to standard virtual calls. Will
                prepare a patch with a few more Klass implementations to
                make it a bit more complete. :)</div>
              <div style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                <br>
              </div>
              <div style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                /Erik</div>
              <br style="font-family:Helvetica; font-size:12px;
                font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
              <blockquote type="cite" style="font-family:Helvetica;
                font-size:12px; font-style:normal; font-variant:normal;
                font-weight:normal; letter-spacing:normal;
                line-height:normal; orphans:auto; text-align:start;
                text-indent:0px; text-transform:none;
                white-space:normal; widows:auto; word-spacing:0px">
                <div style="font-size:12px; font-style:normal;
                  font-variant:normal; font-weight:normal;
                  letter-spacing:normal; line-height:normal;
                  orphans:auto; text-align:start; text-indent:0px;
                  text-transform:none; white-space:normal; widows:auto;
                  word-spacing:0px">
                  thanks,<br>
                  StefanK<br>
                  <br>
                  <br>
                  <blockquote type="cite"><br>
                    Cheers!<br>
                    <br>
                    /Erik</blockquote>
                </div>
              </blockquote>
            </blockquote>
          </div>
          <br>
        </div>
      </div>
    </blockquote>
    <br>
  </body>
</html>