RFR: 8310428: Add CollectedHeap::reserved_range() API to return reserved heap memory range

Mon Jun 26 13:13:18 UTC 2023

On 2023-06-26 10:27, Thomas Stüfe wrote:
> Hi Stefan,
>
> thank you for your answer. Sorry, if I came over as frustrated. I was 
> a bit, but it is not ZGCs fault.
>
> Please find remarks inline.
>
> On Mon, Jun 26, 2023 at 8:37 AM Stefan Karlsson 
> <stefan.karlsson at oracle.com> wrote:
>
>
>
>     On 2023-06-23 22:22, Thomas Stuefe wrote:
>     > On Wed, 21 Jun 2023 15:30:17 GMT, Ashutosh Mehra
>     <duke at openjdk.org> wrote:
>     >
>     >> Please review this patch to add a new API
>     `CollectedHeap::reserved_range()`. The changes are extracted out
>     from https://github.com/openjdk/jdk/pull/14520
>     <https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/14520__;!!ACWV5N9M2RV99hQ!Pva_dFrIXgl9m3x2UtlvRQnCFBQqg_g7851qhlpEBPT-KFatyuvEAVHQPPGF9-sDNMQh_Xry9TgRa1dTnxUKaABAsQk$>
>     as per the suggestion
>     [here](https://github.com/openjdk/jdk/pull/14520#discussion_r1234991165
>     <https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/14520*discussion_r1234991165__;Iw!!ACWV5N9M2RV99hQ!Pva_dFrIXgl9m3x2UtlvRQnCFBQqg_g7851qhlpEBPT-KFatyuvEAVHQPPGF9-sDNMQh_Xry9TgRa1dTnxUKYSs-85M$>).
>     Support for ZGC is not implemented yet as it can have
>     discontiguous heap regions and unlike other collectors, it does
>     not set `CollectedHeap::_reserved`.
>     >> 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.
>     > Please let's have the discussion in the open.
>     >
>     > Nobody answered me, but I repeat, there is merit in knowing the
>     contiguous region for collectors that have a contiguous reserved
>     region.
>
>     Sweden had a public holiday on Friday and people are heading out for
>     vacation. We had to prioritize other discussions on Thursday.
>
>     > I don't understand why we have to treat developers like children
>     and force them to use really bad workarounds.
>
>     What? There is a trade off between adding the wrong abstraction and
>     cleaning up other parts of the code.
>
>
> 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.

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.

>
> Thinking about this further, maybe we can find a better compromise, 
> see below.
>
>
>     >
>     > 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:
>     https://github.com/openjdk/jdk/blob/bfcca5eff96ac3cd72996b6c4865872c2da4de53/src/hotspot/share/memory/metaspace.cpp#L794-L796
>     <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$>.
>     >
>     > 1) we don't know the address range of the heap
>     > 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.
>     > 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.
>     > 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:
>     https://github.com/openjdk/jdk/blob/bfcca5eff96ac3cd72996b6c4865872c2da4de53/src/hotspot/share/memory/virtualspace.cpp#L548-L558
>     <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$>
>     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.
>     > 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.
>     >
>     > And all this complexity for a scenario that is not even
>     supported on ZGC (CompressedOops) !
>
>     I think this is an unfair assessment. The code grew to what it is
>     today
>     even when we had the CollectedHeap::reserved_range. I don't think we
>     should blame ZGC that this hasn't been cleaned up.
>
>
> Okay, that is true. But cleaning up this code would be easier with 
> heap cooperation.
>
>
>     >
>     > 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§
>     >
>     > So,no, I don't think this discussion has come to a satisfying close.
>
>     The door isn't closed for adding an API that helps clean up those
>     parts.
>     Could you add the required heap range to CompressedOops and used that
>     for all the bullets above? 
>
>
> 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.:
> - in Shenandoah, certain helper structures are attempt-reserved in 
> lower ranges
> - 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).
>
> 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).
>
> 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.
>
> 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.

This was yet another API that we removed from CollectedHeap:
https://mail.openjdk.org/pipermail/hotspot-dev/2019-August/039076.html

If we were to reintroduce this API there needs to be a good and fast 
implementation for ZGC.

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.

StefanK

>     If not, and we really need to add this to
>     CollectedHeap for some reason, then I think reserved_range need to be
>     renamed to something that is bleeding obvious not going to be
>     available
>     for all GCs.
>
>     StefanK
>
>
> Thomas
>
>     >
>     > -------------
>     >
>     > PR Comment:
>     https://git.openjdk.org/jdk/pull/14595#issuecomment-1604898333
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-dev/attachments/20230626/7751cd02/attachment-0001.htm>