<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body>
    <br>
    <br>
    <div class="moz-cite-prefix">On 2023-06-26 10:27, Thomas Stüfe
      wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:CAA-vtUxwPsQj90h22gFBfMk2gy-3D9G-ZjaAe2Pdo7OV=4E4rg@mail.gmail.com">
      
      <div dir="ltr">
        <div dir="ltr">Hi Stefan,<br>
        </div>
        <div><br>
        </div>
        <div>thank you for your answer. Sorry, if I came over as
          frustrated. I was a bit, but it is not ZGCs fault.<br>
        </div>
        <div><br>
        </div>
        <div>Please find remarks inline.<br>
        </div>
        <div><br>
        </div>
        <div class="gmail_quote">
          <div dir="ltr" class="gmail_attr">On Mon, Jun 26, 2023 at
            8:37 AM Stefan Karlsson <<a href="mailto:stefan.karlsson@oracle.com" moz-do-not-send="true" class="moz-txt-link-freetext">stefan.karlsson@oracle.com</a>>
            wrote:<br>
          </div>
          <blockquote class="gmail_quote" style="margin:0px 0px 0px
            0.8ex;border-left:1px solid
            rgb(204,204,204);padding-left:1ex"><br>
            <br>
            On 2023-06-23 22:22, Thomas Stuefe wrote:<br>
            > On Wed, 21 Jun 2023 15:30:17 GMT, Ashutosh Mehra <<a href="mailto:duke@openjdk.org" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">duke@openjdk.org</a>>
            wrote:<br>
            ><br>
            >> Please review this patch to add a new API
            `CollectedHeap::reserved_range()`. The changes are extracted
            out from <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/14520__;!!ACWV5N9M2RV99hQ!Pva_dFrIXgl9m3x2UtlvRQnCFBQqg_g7851qhlpEBPT-KFatyuvEAVHQPPGF9-sDNMQh_Xry9TgRa1dTnxUKaABAsQk$" rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/openjdk/jdk/pull/14520</a>
            as per the suggestion [here](<a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/14520*discussion_r1234991165__;Iw!!ACWV5N9M2RV99hQ!Pva_dFrIXgl9m3x2UtlvRQnCFBQqg_g7851qhlpEBPT-KFatyuvEAVHQPPGF9-sDNMQh_Xry9TgRa1dTnxUKYSs-85M$" rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/openjdk/jdk/pull/14520#discussion_r1234991165</a>).
            Support for ZGC is not implemented yet as it can have
            discontiguous heap regions and unlike other collectors, it
            does not set `CollectedHeap::_reserved`.<br>
            >> I've had an off-line discussion with @fisk and
            created an investigative RFE based on his ideas. Please see
            #14520 and use that PR for further discussion on this topic.<br>
            > Please let's have the discussion in the open.<br>
            ><br>
            > Nobody answered me, but I repeat, there is merit in
            knowing the contiguous region for collectors that have a
            contiguous reserved region.<br>
            <br>
            Sweden had a public holiday on Friday and people are heading
            out for <br>
            vacation. We had to prioritize other discussions on
            Thursday.<br>
            <br>
            > I don't understand why we have to treat developers like
            children and force them to use really bad workarounds.<br>
            <br>
            What? There is a trade off between adding the wrong
            abstraction and <br>
            cleaning up other parts of the code.<br>
          </blockquote>
          <div><br>
          </div>
          <div>My original point was that the abstraction is correct for
            most GCs. An API like the proposed one - with clear
            documentation, a clear "not supported" return code the dev
            has to handle, and a companion capability query API - would
            hopefully not be misused. It would also mimic what we do on
            a platform level.<br>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    Sure. If the name can convey that there's a limitation and that not
    all GCs support it, then I think it is OK. Though, if we add
    something like this, I'd like to see how that API is going to be
    used and how support for that feature will look like for those GCs
    that don't support the API.<br>
    <br>
    <blockquote type="cite" cite="mid:CAA-vtUxwPsQj90h22gFBfMk2gy-3D9G-ZjaAe2Pdo7OV=4E4rg@mail.gmail.com">
      <div dir="ltr">
        <div class="gmail_quote">
          <div><br>
          </div>
          <div>Thinking about this further, maybe we can find a better
            compromise, see below.<br>
          </div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0px 0px 0px
            0.8ex;border-left:1px solid
            rgb(204,204,204);padding-left:1ex">
            <br>
            ><br>
            > Just an example of the weird complexity this API
            omission causes. See class space reservation, where we try
            to reserve class space adjacent to the java heap: <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/bfcca5eff96ac3cd72996b6c4865872c2da4de53/src/hotspot/share/memory/metaspace.cpp*L794-L796__;Iw!!ACWV5N9M2RV99hQ!Pva_dFrIXgl9m3x2UtlvRQnCFBQqg_g7851qhlpEBPT-KFatyuvEAVHQPPGF9-sDNMQh_Xry9TgRa1dTnxUKSj-O4G4$" rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/openjdk/jdk/blob/bfcca5eff96ac3cd72996b6c4865872c2da4de53/src/hotspot/share/memory/metaspace.cpp#L794-L796</a>.<br>
            ><br>
            > 1) we don't know the address range of the heap<br>
            > 2) so we have to misuse CompressedOops::encoding_range,
            which is a bad fit. Encoding range is not heap range. It can
            (often does) start before the heap, so we don't know the
            heap start address. It only tells us the heap range end, but
            that is an accident of implementation: semantically, it is
            incorrect since the encoding range end is not the end of the
            heap.<br>
            > 3) Since we don't get the start address, we only know
            heap range end. That forces us to reserve the class space
            adjacent - following - the heap range.<br>
            > 4) So, for zero-based heap reservation, we have to
            leave a (potentially very large - 1G) gap between zero-based
            encoding range end and the heap end: <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/bfcca5eff96ac3cd72996b6c4865872c2da4de53/src/hotspot/share/memory/virtualspace.cpp*L548-L558__;Iw!!ACWV5N9M2RV99hQ!Pva_dFrIXgl9m3x2UtlvRQnCFBQqg_g7851qhlpEBPT-KFatyuvEAVHQPPGF9-sDNMQh_Xry9TgRa1dTnxUKsZdcJto$" rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/openjdk/jdk/blob/bfcca5eff96ac3cd72996b6c4865872c2da4de53/src/hotspot/share/memory/virtualspace.cpp#L548-L558</a>
            This is bad, since it reduces the chance of getting
            zero-based heap. It also means we need to code intimate
            knowledge about class space (e.g. class space size) at heap
            reservation. Heap reservation should not have to know these
            things.<br>
            > 5) if, instead, we had known the start address of the
            heap at (4), we could just reserve the class space in front
            to it. There would have to be no need for leaving a large
            gap between zero-based compressed oops encoding range end
            and heap end. Also, we would not need to have class-space
            knowledge at heap reservation.<br>
            ><br>
            > And all this complexity for a scenario that is not even
            supported on ZGC (CompressedOops) !<br>
            <br>
            I think this is an unfair assessment. The code grew to what
            it is today <br>
            even when we had the CollectedHeap::reserved_range. I don't
            think we <br>
            should blame ZGC that this hasn't been cleaned up.<br>
          </blockquote>
          <div><br>
          </div>
          <div>Okay, that is true. But cleaning up this code would be
            easier with heap cooperation.<br>
          </div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0px 0px 0px
            0.8ex;border-left:1px solid
            rgb(204,204,204);padding-left:1ex">
            <br>
            ><br>
            > This is suboptimal. With a simple
            "CollectedHeap::reserved_range()" in combination with
            "CollectedHeap::is_contiguous()", this could be solved in
            about 10 lines of code.x§<br>
            ><br>
            > So,no, I don't think this discussion has come to a
            satisfying close.<br>
            <br>
            The door isn't closed for adding an API that helps clean up
            those parts. <br>
            Could you add the required heap range to CompressedOops and
            used that <br>
            for all the bullets above? </blockquote>
          <div><br>
          </div>
          <div>Thinking about this further, I'd like to remove
            consideration of CompressedOops from klass range reservation
            completely. There is no connection anymore. And this problem
            is not limited to klass reservation. Reserving memory within
            a range of preferred addresses is done for other cases too,
            nor is it limited to lower address ranges. Heap reservation
            itself, for instance. Also, e.g.:<br>
          </div>
          <div>- in Shenandoah, certain helper structures are
            attempt-reserved in lower ranges</div>
          <div>- We could increase the chance of CDS runtime attaching
            the archive at the preferred base address in the face of
            ASLR by splitting the reservation of CDS archive (which has
            to be at address X) and the one of the follow-up class space
            (which has to be just within klass encoding range).</div>
          <br>
          <div>I would like to have something like
            "os::reserve_in_range(from, to, side-conditions)". Such an
            API would not need to know anything about the heap but would
            benefit from knowing large reserved areas *like* the heap to
            prevent unnecessary work. Typically the heap is the largest
            area at that point in time. (Pity that we cannot use NMT for
            this).</div>
          <div><br>
          </div>
          I was experimenting with such an API. My idea was to use, on
          supported platforms, OS information to find an address hole
          (procfs, VirtualQuery), otherwise to fall back to
          ladder-probing the address space with mmap. The latter can be
          sped up considerably by omitting the mmap calls for regions we
          know are reserved. <br>
          <div><br>
          </div>
          <div>A compromise could be adding an API like
            "is_in_reserved()" to CollectedHeap. That would not be
            ideal, we'd still be looping, but the expensive part - the
            mmaps - could be avoided. Hopefully, such an API could be
            implemented in a fast way even for ZGC. Such an API would
            not assume a contiguous heap.<br>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    This was yet another API that we removed from CollectedHeap:<br>
<a class="moz-txt-link-freetext" href="https://mail.openjdk.org/pipermail/hotspot-dev/2019-August/039076.html">https://mail.openjdk.org/pipermail/hotspot-dev/2019-August/039076.html</a><br>
    <br>
    If we were to reintroduce this API there needs to be a good and fast
    implementation for ZGC.<br>
    <br>
    Maybe there are ways to rearrange the initialization order so that
    sub-systems that need to perform particular memory area reservation
    can be done before we reserve the heap? FWIW, ZGC can reserve
    discontiguous memory (See:
    ZVirtualMemoryManager::reserve_discontiguous) and can deal with
    memory areas being pre-reserved by other sub-systems.<br>
    <br>
    StefanK<br>
    <br>
    <blockquote type="cite" cite="mid:CAA-vtUxwPsQj90h22gFBfMk2gy-3D9G-ZjaAe2Pdo7OV=4E4rg@mail.gmail.com">
      <div dir="ltr">
        <div class="gmail_quote">
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0px 0px 0px
            0.8ex;border-left:1px solid
            rgb(204,204,204);padding-left:1ex">If not, and we really
            need to add this to <br>
            CollectedHeap for some reason, then I think reserved_range
            need to be <br>
            renamed to something that is bleeding obvious not going to
            be available <br>
            for all GCs.<br>
            <br>
          </blockquote>
        </div>
        <div class="gmail_quote">
          <blockquote class="gmail_quote" style="margin:0px 0px 0px
            0.8ex;border-left:1px solid
            rgb(204,204,204);padding-left:1ex">
            StefanK<br>
            <br>
          </blockquote>
          <div><br>
          </div>
          <div>Thomas<br>
          </div>
          <div> </div>
          <blockquote class="gmail_quote" style="margin:0px 0px 0px
            0.8ex;border-left:1px solid
            rgb(204,204,204);padding-left:1ex">
            ><br>
            > -------------<br>
            ><br>
            > PR Comment: <a href="https://git.openjdk.org/jdk/pull/14595#issuecomment-1604898333" rel="noreferrer" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">https://git.openjdk.org/jdk/pull/14595#issuecomment-1604898333</a><br>
            <br>
          </blockquote>
        </div>
      </div>
    </blockquote>
    <br>
  </body>
</html>