Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-7102470

RFE: USDT1 versus USDT2 ifdefing should be revisited

XMLWordPrintable

    • Icon: Enhancement Enhancement
    • Resolution: Not an Issue
    • Icon: P5 P5
    • tbd
    • hs23
    • hotspot
    • svc
    • generic
    • os_x

      Comment from Tom R:

      > On the issue of the dtrace changes, couldn't we minimize later
      > disruption by moving the USDT1/USDT2 distinction into a header file?
      > For instance, instead of doing this:
      >
      > +#ifndef USDT2
      >
      > HS_DTRACE_PROBE1(hotspot, thread__sleep__end,0);
      >
      > +#else /* USDT2 */
      > + HOTSPOT_THREAD_SLEEP_END(
      > + 0);
      > +#endif /* USDT2 */
      >
      > Couldn't it just have
      >
      > HOTSPOT_THREAD_SLEEP_END(0);
      >
      > with this ifdef in a header somewhere?
      >
      > #ifndef USDT2
      > #define HOTSPOT_THREAD_SLEEP_END(arg) \
      > HS_DTRACE_PROBE1(hotspot, thread__sleep__end, (arg))
      > #endif /* USDT2 */
      >
      > It minimizes the ugly until we resolve the real issue. I guess
      > the PROBE_DECL stuff will have to be somewhere else too. Anyway,
      > I'm fine with pushing it as is but I wanted to at least mention this.


      My reply to Tom R:

      > I'll include this in the new bug as a way to phase in the
      > changes from USDT1 -> USDT2. I don't want to make that
      > change now since it will make it more difficult to keep
      > in sync with the MacOS X port project.


      A reply from Paul H on Tom R's thread:

      > Do we really need to keep in sync with the old macosx-port project?
      > Afaic, we're just using that code as a starting point for this project.
      >
      > I second Tom's suggestion to move the USDT1/2 distinction into
      > a header file. Doing so would make this changeset a whole lot smaller.


      My reply to Paul H:

      On 10/12/11 4:00 PM, Daniel D. Daugherty wrote:
      > On 10/12/11 3:19 PM, Paul Hohensee wrote:
      >> Do we really need to keep in sync with the old macosx-port project?
      >
      > Yes. That's one of the ways that I'm keeping this port project sane.
      > As far as I know, the bsd-port/hotspot and macosx-port/hotspot repos
      > have not yet been retired. If I have to resync with either of those
      > repos again before they are retired, then I would like that to go
      > smoothly.
      >
      >
      >> Afaic, we're just using that code as a starting point for this project.
      >
      > 'Afaic' or 'Afaik'? (care or know?)
      >
      > Yes, we're using that code as a starting point for this project, but
      > I don't yet have testing up and running on MacOS X so I don't want to
      > make non-trivial MacOS X changes blind.
      >
      >
      >> I second Tom's suggestion to move the USDT1/2 distinction into
      >> a header file. Doing so would make this changeset a whole lot smaller.
      >
      > Not going to happen in this changeset. I will file another bug to do
      > more work on USDT1/2 stuff, but I don't yet have a way to test making
      > a change like that on MacOS X.


      Paul's reply to me:

      On 10/12/11 4:07 PM, Paul Hohensee wrote:
      > On 10/12/11 6:00 PM, Daniel D. Daugherty wrote:
      >> On 10/12/11 3:19 PM, Paul Hohensee wrote:
      >>> Do we really need to keep in sync with the old macosx-port project?
      >>
      >> Yes. That's one of the ways that I'm keeping this port project sane.
      >> As far as I know, the bsd-port/hotspot and macosx-port/hotspot repos
      >> have not yet been retired. If I have to resync with either of those
      >> repos again before they are retired, then I would like that to go
      >> smoothly.
      >
      > Ok. Maybe someone can tell me when they'll be retired, or if they're
      > relevant to a jdk7 port. I see what I can find out.
      >
      >>
      >>
      >>> Afaic, we're just using that code as a starting point for this project.
      >>
      >> 'Afaic' or 'Afaik'? (care or know?)
      >
      > "concerned". :)
      >
      >>
      >> Yes, we're using that code as a starting point for this project, but
      >> I don't yet have testing up and running on MacOS X so I don't want to
      >> make non-trivial MacOS X changes blind.
      >>
      >>
      >>> I second Tom's suggestion to move the USDT1/2 distinction into
      >>> a header file. Doing so would make this changeset a whole lot smaller.
      >>
      >> Not going to happen in this changeset. I will file another bug to do
      >> more work on USDT1/2 stuff, but I don't yet have a way to test making
      >> a change like that on MacOS X.
      >
      > Reluctantly, Ok.
      Comment from David H:

      > src/share/vm/prims/jni.cpp
      >
      > The USDT2 ifdef changes are truly ugly especially in this file.
      > Is there no way to hide USDT1 vs USDT2 at a lower level so that
      > the top-level probe points are not affected? I know this is
      > flagged for "future work" but history shows such cleans ups
      > rarely if ever happen - and this current code really is pretty
      > awful to look at.


      My reply to David H:

      > Yes, the #ifdef'ing makes the current code awful. I don't think
      > anyone will dispute that observation.
      >
      > The better solution would be for Solaris to migrate to USDT2.
      > I'm discussing that with Keith M. However, I don't want
      > to take much of his time right now because he is hip deep in
      > another alligator swamp at the moment.
      >
      > The best I can offer at the moment is filing a new bug to track
      > the future work. Yes, I know that clean up type work rarely gets
      > done, but this merge with the MacOS X port project needs to get
      > into HSX-23 as soon as possible. Broken functionality in the
      > other platforms is a stopper, but ugly is not a stopper.
      Comment from Vladimir K:

      > Why new Dtrace implementation for MacOS X use 2 lines where it is not
      > needed (a lot of places)? For example:
      >
      > +#else /* USDT2 */
      > + HOTSPOT_GC_END(
      > +);
      > +#endif /* USDT2 */
      >
      > Is this what you called "does not follow HotSpot style guidelines very consistently"?


      A reply to Vladimir's comment from Paul H:

      > I'm ok with fixing that (the style issue) later, as long as it does
      > get fixed. Use of the macro preprocessor is the devil, unless we
      > absolutely, positively have no choice. :)


      And my reply to Paul H's comment:

      > I can promise that a bug will get filed to track the whole issue
      > of migrating from USDT1 -> USDT2. Included in that bug will be
      > a note about the style issues. That's the best I can say at the
      > moment.


      Roger H's reply to Vladimir K's comment?

      > I did the USDT2 implementation with some mechanical tools and a lot
      > of by-hand followup. My intension was to leave the structure of the
      > USDT1 macro calls in place. Most of the USDT2 changes are a smallish
      > delta on the USDT1 macro calls. If we were to have macro-in-a-macro
      > capabilities, we could implement this small difference with another macro.
      >
      > Fortunately for sanity's sake, macros-in-macros don't work well in the C
      > preprocessor, and I had to duplicate the entire macro call. If Oracle
      > moves the Solaris implementation from USDT1 to USDT2, then by all means
      > the USDT2 code should be cleaned up. On the other hand, if the USDT1
      > implementation is to live on, any formatting changes should enhance the
      > ability to make consistent modifications to both USDT1 and USDT2.


      Closing comment from Vladimir K:

      > Thank you, Dan
      >
      > I agree for this round we should leave new Dtrace amd Mikefiles code
      > as it is. Thank you, Alexander and Roger for explaining these changes.


      Closing comment from Paul H:

      > Ditto for me.


      Reply to Roger H's comment from Keith M:

      > FWIW, I believe the build platform for jdk7 is Solaris 10, which does
      > support USDT2, as far as I know. So if we can convert to use USDT2
      > all the time and drop the USDT1 code, that would be good.
      The MacOS X Port project used the following changeset for the initial
      push from macosx-port/hotspot -> HSX-23:

          http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/436b4a3231bf

          7098194: integrate macosx-port changes

      and that changeset includes a newer Dtrace implementation along side
      the original Dtrace implementation. The code is #ifdef'ed in the
      following forms:

              #ifndef USDT2
      <older Dtrace implementation is in this block>
      <some non-Dtrace code (macros) that call the older Dtrace>
      <implementation are also in this block>
              #else /* USDT2 */
      <new Dtrace implementation for MacOS X in this block>
              #endif /* USDT2 */

              #ifndef USDT2
      <older Dtrace implementation is in this block>
      <no #else because the newer Dtrace doesn't need/have the equivalent>
              #endif /* ! USDT2 */

      The purpose of this RFE is to request that the #ifdef'ing be
      revisited to address comments from the code reviewers of 7098194.

            Unassigned Unassigned
            dcubed Daniel Daugherty
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

              Created:
              Updated:
              Resolved:
              Imported:
              Indexed: