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:

> > > 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.

Your proposal increases complexity for the benefit of a rather small corner case; my proposal (disregarding heap specifics completely and just attempting ladder-attaching in the 32g range) would decrease complexity while improving placement chances below 32g a lot more. Are there any reasons why this would not work? If not, why not choose the better approach? If its just about backporting, I do not think that should dictate what solution we choose. Rather implement the small solution in 11u directly then.

I also do not think there is any time pressure here. So we can afford us the luxury to do things right instead of rushing it.

> 
> 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.
> 

Is that not what I wrote? I quote myself:
>> This coding returns true if the given value is usable as a _narrow klass base_.   
>> <snip> 
>> 2G would be valid as start of ccs though. But that distinction would have to be clearly coded out, atm its not. See metaspace.cpp:717 and :739. But I am not sure if its worth the complexity.

So the function is correct. The usage is incorrect.

> 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.

Sure. This is my error, slipped in with the CDS remodelling. Feel free to file a bug report about this and put it on my name.

Cheers, Thomas

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

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