RFR: 8258576: Try to get zerobased CCS if heap is above 32 and CDS is disabled [v2]

[Thomas Stuefe]

On Fri, 18 Dec 2020 15:47:12 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

>> One other aspect to check, if ccs is placed below the heap, is whether it gets placed in the way of the sbrk. Important for platforms which implement malloc via sbrk, because you could get malloc fails later. It is the case for AIX and Solaris at least (the latter matters when downporting). For AIX, I added safety coding right into os::reserve_memory. It may also be an issue on MacOS, that should be tested. I believe glibc is fine.
>
>> 
>> 
>> > Hm, I don't understand that one. OopEncodingHeapMax is normally 32G. CompressedOops.end() is the end of the coops heap; probably way higher than 32G. So you will get an underflow in the substraction, won't you?
>> 
>> It should not be beyond 32G if heap was places below 32G, no?
>> 
> 
> Ok, I see, but this is not the point of the enhancement. The point here is to
> make use of the lower 32G if the heap is above that limit.
> 
> And I'd like to keep that scope. I'm planning to downport the small change to 11u
> where CDS is off by default.
> 
> Of course you can further optimize this in a follow-up.
> 
>> But the more I look at this coding the more I think this should not depend on heap and UseCompressedOops at all. UseCompressedOops does not really matter.
> 
> That observation is correct. I wanted to eliminate the dependency but
> unfortunately I haven't found something in CollectedHeap that takes the noaccess
> prefix required for the not-zerobased coops heap into account (the noaccess
> prefix is a protected page used for implicit null
> checks). CompressedOops::base() does.
> 
>> And note that the low address does not have to be HeapBaseMinAddress either. ccs can live anywhere. The dependence on HeapBaseMinAddress is incidental.
> 
> I think the naming is incidental. AFAIK the intend is not to use the address space below HeapBaseMinAddress.
> 
>> > > It would be interesting to know why aarch64 and OSX don't use shift 0 in the new test case.
>> > 
>> > 
>> > HeapBaseMinAddress seems to be 2G on all platforms. On AARCH64 2G is considered invalid because it is not 4G aligned. I must admit that I don't understand this. Below 32G the CCS will be zerobased. There is no need to load the base address efficiently into a register. I think that part could be removed, don't you think that too?
>> > ```
>> > bool CompressedKlassPointers::is_valid_base(address p) {
>> > #ifdef AARCH64
>> >   // Below 32G, base must be aligned to 4G.
>> >   // Above that point, base must be aligned to 32G
>> >   if (p < (address)(32 * G)) {
>> >     return is_aligned(p, 4 * G);
>> >   }
>> >   return is_aligned(p, (4 << LogKlassAlignmentInBytes) * G);
>> > #else
>> >   return true;
>> > #endif
>> > }
>> > ```
>> 
>> No, this coding is correct. See comment above the function.
> 
> The comment says
> 
> // Given an address p, return true if p can be used as an encoding base.
> //  (Some platforms have restrictions of what constitutes a valid base address).
> 
> But the parameter passed is not the encoding base. It is the desired beginning
> of ccs. If ccs is mapped at 2G then the encoding base is 0 (actually it is 0 if
> the ccs end is <= KlassEncodingMetaspaceMax, see
> CompressedKlassPointers::initialize).  So either the function or its users are
> not correct.
> 
> In other words: mapping ccs at 2G would result in a zerobased ccs. I'd be extremely
> surprised if 0x0 as encoding base is actually invalid on aarch64. IMHO opinion
> it is a bug that it is not allowed to map ccs at 2g on aarch64.

> One other aspect to check, if ccs is placed below the heap, is whether it gets placed in the way of the sbrk. Important for platforms which implement malloc via sbrk, because you could get malloc fails later. It is the case for AIX and Solaris at least (the latter matters when downporting). For AIX, I added safety coding right into os::reserve_memory. It may also be an issue on MacOS, that should be tested. I believe glibc is fine.

Thinking this through, I believe we are covered here - or at least this patch won't introduce any problems we don't already have - since we already have code paths attaching at HeapBaseMinAddress. 

It might be good to file a future RFE to extend the AIX workaround (which prevents attaching to a configurable safety zone above brk) to all platforms. Just on the off-chance that someone (libc or third party code) uses sbrk() for memory allocation. But that is a separate issue.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1815