<html>
  <head>
    <meta content="text/html; charset=windows-1252"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <br>
    <div class="moz-cite-prefix">On 2014-10-08 15:11, Erik Österlund
      wrote:<br>
    </div>
    <blockquote cite="mid:D9D9A71E-42BC-40B0-8A24-6CD9E1E217E1@lnu.se"
      type="cite">
      <meta http-equiv="Content-Type" content="text/html;
        charset=windows-1252">
      <div style="word-wrap:break-word">Hi Stefan,
        <div><br>
          <div>
            <div>On 08 Oct 2014, at 14:30, 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">
            <blockquote type="cite">
              <div bgcolor="#FFFFFF">Hi Erik,<br>
                <br>
                <div class="x_moz-cite-prefix">On 2014-10-08 13:20, Erik
                  Österlund wrote:<br>
                </div>
                <blockquote type="cite">
                  <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 moz-do-not-send="true"
                  class="x_moz-txt-link-freetext"
                  href="http://cr.openjdk.java.net/%7Estefank/eriko/8059936/webrev.00/">
http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.00/</a><br>
                bug: <a moz-do-not-send="true"
                  class="x_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
</pre>
              </div>
            </blockquote>
            <div>Here's a new take where I declare my enum on that line
              with a compiler helmet on. The new archive contains the
              _incremental and _full patch to fix this issue.</div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01.delta/">http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01.delta/</a><br>
    <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/">http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/</a><br>
    <br>
    <blockquote cite="mid:D9D9A71E-42BC-40B0-8A24-6CD9E1E217E1@lnu.se"
      type="cite">
      <div style="word-wrap:break-word">
        <div>
          <div>
            <div>There's no way I can run JPRT myself is there?</div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    Unfortunately, no.<br>
    <br>
    This patch fails on some platforms that don't use precompiled
    headers. Can you compile with USE_PRECOMPILED_HEADER=0 and take care
    of the compile errors?<br>
    <br>
    I saw that one of those failures happened after
    specialized_oop_closures.inline.hpp was included from oop.hpp:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/src/share/vm/oops/oop.hpp.udiff.html">http://cr.openjdk.java.net/~stefank/eriko/8059936/webrev.01/src/share/vm/oops/oop.hpp.udiff.html</a><br>
    <br>
    You need to rework you dependencies so that you don't include
    .inline.hpp files from .hpp files.<br>
    <br>
    See the new Files section of:<br>
    <a class="moz-txt-link-freetext" href="https://wiki.openjdk.java.net/display/HotSpot/StyleGuide">https://wiki.openjdk.java.net/display/HotSpot/StyleGuide</a><br>
    <br>
    cheers,<br>
    StefanK<br>
    <br>
    <blockquote cite="mid:D9D9A71E-42BC-40B0-8A24-6CD9E1E217E1@lnu.se"
      type="cite">
      <div style="word-wrap:break-word">
        <div>
          <div>
            <div><br>
            </div>
            <div>Thank you,</div>
            <div><br>
            </div>
            <div>/Erik</div>
            <div><br>
            </div>
          </div>
        </div>
      </div>
      <div style="word-wrap:break-word">
        <div>
          <div>
            <div><br>
            </div>
            <blockquote type="cite">
              <div bgcolor="#FFFFFF">
                <pre>StefanK
</pre>
                <br>
                <blockquote 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_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_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_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>
              </div>
            </blockquote>
          </div>
          <br>
        </div>
      </div>
    </blockquote>
    <br>
  </body>
</html>