RFR(L): 8064457: Introduce compressed oops mode "disjoint base" and improve compressed heap handling.

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Wed Dec 3 13:49:30 UTC 2014

Hi Vladimir,

thanks for looking at the change!  See my comments inline below.

I made a new webrev: 
Incremental changes:

Best regards,

> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
> Sent: Mittwoch, 3. Dezember 2014 00:46
> To: Lindenmaier, Goetz; 'hotspot-dev at openjdk.java.net'
> Subject: Re: RFR(L): 8064457: Introduce compressed oops mode "disjoint base" and improve compressed heap handling.
> This looks good to me. Someone in runtime/gc have to look on it too.
> universe.cpp about SystemProperty("com.sap.vm.test.compressedOopsMode"
> we have:
> java.vm.info=mixed mode, sharing
> so we can have:
Yes, that's good for me. Fixed.

> I am not expert in properties names but I don't want to have 'com.sap' 
> in VM's property name.

> virtualspace.cpp:
>Could you fix release() - it does not reset _alignment?

> In try_reserve_heap(), please, use (base == NULL) instead of (!base). 
> And you don't need 'return;' in alignment check at the end of method.

> In initialize_compressed_heap() again (!_base). 

> You don't stop (check 
> (base == NULL)) after successful unscaled, zerobased, disjointbase 
> allocations. You need to separate them with the check:
> +
> +    }
> +  }
> +  if (_base == NULL) {
> +
> +    if (PrintCompressedOopsMode && Verbose) {
> +      tty->print(" == Z E R O B A S E D ==\n");
> +    }
> and so on.
No, I can't and don't want to check for _base != NULL.
I always keep the result of the last try, also if it didn't fulfil the required properties.
So I take that result and go into the next check.  That check might succeed 
with the heap allocated before.
This allows me to separate allocation and placement criteria, and to have the 
placement criteria checked in only one place (per mode).
Only for HeapBaseMinAddress I don't do it that way, I explicitly call release().
This way I can enforce mode heapbased.

> num_attempts calculation and while() loop are similar in unscaled and 
> zerobased cases. Could you move it into a separate method?
I can do that, but I don't like it as I have to pass in 7 parameters.
That makes the code not much more readable.  The function will look like this:

void ReserveHeapSpace::try_reserve_range(char *const highest_start, char *lowest_start, size_t attach_point_alignment,
                                         char *aligned_HBMA, size_t size, size_t alignment, bool large) {  
  guarantee(HeapSearchSteps > 0, "Don't set HeapSearchSteps to 0");
  const size_t attach_range = highest_start - lowest_start;  
  // Cap num_attempts at possible number. 
  // At least one is possible even for 0 sized attach range.
  const uint64_t num_attempts_possible = (attach_range / attach_point_alignment) + 1;
  const uint64_t num_attempts_to_try   = MIN2(HeapSearchSteps, num_attempts_possible);
  const size_t stepsize = align_size_up(attach_range / num_attempts_to_try, attach_point_alignment);
  // Try attach points from top to bottom.
  char* attach_point = highest_start;
  while (attach_point >= lowest_start  &&
         attach_point <= highest_start &&  // Avoid wrap around.
         (!_base || _base < aligned_HBMA || _base + size > (char *)UnscaledOopHeapMax)) {
    try_reserve_heap(size, alignment, large, attach_point);
    attach_point -= stepsize;

> In disjointbase while() condition no need for _base second check:
> +           (_base == NULL ||
> +            ((_base + size > (char *)OopEncodingHeapMax) &&
I need this for the same reason as above:  This is the check for successful allocation.


On 11/21/14 5:31 AM, Lindenmaier, Goetz wrote:
> Hi,
> I prepared a new webrev trying to cover all the issues mentioned below.
> http://cr.openjdk.java.net/~goetz/webrevs/8064457-disjoint/webrev.01/
> I moved functionality from os.cpp and universe.cpp into
> ReservedHeapSpace::initialize_compressed_heap().
> This class offers to save _base and _special, which I would have to reimplement
> if I had improved the methods I had added to os.cpp to also allocate large page
> heaps.
> Anyways, I think this class is the right place to gather code trying to reserve
> the heap.
> Also, I get along without setting the shift, base, implicit_null_check etc. fields
> of Universe, so there is no unnecessary calling back and forth between the two
> classes.
> Universe gets the heap back, and then sets the properties it needs to configure
> the compressed oops.
> All code handling the noaccess prefix is in a single method, too.
> Best regards,
>    Goetz.
> Btw, I had to workaround a SS12u1 problem: it wouldn't compile
> char * x = (char*)UnscaledOopHeapMax - size in 32-bit mode.
> -----Original Message-----
> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Lindenmaier, Goetz
> Sent: Montag, 17. November 2014 09:33
> To: 'Vladimir Kozlov'; 'hotspot-dev at openjdk.java.net'
> Subject: RE: RFR(L): 8064457: Introduce compressed oops mode "disjoint base" and improve compressed heap handling.
> Hi Vladimir,
>> It is very significant rewriting and it takes time to evaluate it.
> Yes, I know ... and I don't want to push, but nevertheless a ping
> can be useful sometimes. Thanks a lot for looking at it.
>> And I would not say it is simpler then before :)
> If I fix what you propose it's gonna get even more simple ;)
>> These is what I found so far.
>> The idea to try to allocate in a range instead of just below
>> UnscaledOopHeapMax or OopEncodingHeapMax is good. So I would ask to do
>> several attempts (3?) on non_PPC64 platforms too.
> Set to 3.
>> It is matter of preference but I am not comfortable with switch in loop.
>> For me sequential 'if (addr == 0)' checks is simpler.
> I'll fix this.
>> One thing worries me that you release found space and try to get it
>> again with ReservedHeapSpace. Is it possible to add new
>> ReservedHeapSpace ctor which simple use already allocated space?
> This was to keep diff's small, but I also think a new constructor is good.
> I'll fix this.
>> The next code in ReservedHeapSpace() is hard to understand ():
>> (UseCompressedOops && (requested_address == NULL ||
> requested_address+size > (char*)OopEncodingHeapMax) ?
>> may be move all this into noaccess_prefix_size() and add comments.
> I have to redo this anyways if I make new constructors.
>> Why you need prefix when requested_address == NULL?
> If we allocate with NULL, we most probably will get a heap where
> base != NULL and thus need a noaccess prefix.
>> Remove next comment in universe.cpp:
>> // SAPJVM GL 2014-09-22
> Removed.
>> Again you will release space so why bother to include space for classes?:
>> +          // For small heaps, save some space for compressed class pointer
>> +          // space so it can be decoded with no base.
> This was done like this before.  We must assure the upper bound of the
> heap is low enough that the compressed class space still fits in there.
> virtualspace.cpp
>> With new code size+noaccess_prefix could be requested. But later it is
>> not used if WIN64_ONLY(&& UseLargePages) and you will have empty
>> non-protected page below heap.
> There's several points to this:
>   * Also if not protectable, the heap base has to be below the real start of the
>      heap.  Else the first object in the heap will be compressed to 'null'
>      and decompression will fail.
>   * If we don't reserve the memory other stuff can end up in this space. On
>      errors, if would be quite unexpected to find memory there.
>   * To get a heap for the new disjoint mode I must control the size of this.
>       Requesting a heap starting at (aligned base + prefix) is more likely to fail.
>   * The size for the prefix must anyways be considered when deciding whether the
>       heap is small enough to run with compressed oops.
> So distinguishing the case where we really can omit this would require
> quite some additional checks everywhere, and I thought it's not worth it.
> matcher.hpp
>> Universe::narrow_oop_use_implicit_null_checks() should be true for such
>> case too. So you can add new condition with || to existing ones. The
>> only condition you relax is base != NULL. Right?
> Yes, that's how it's intended.
> arguments.* files
>> Why you need PropertyList_add changes.
> Oh, the code using it got lost.  I commented on this in the description in the webrev.
> "To more efficiently run expensive tests in various compressed oop modes, we set a property with the mode the VM is running in. So far it's called "com.sap.vm.test.compressedOopsMode" better suggestions are welcome (and necessary I guess). Our long running tests that are supposed to run in a dedicated compressed oop mode check this property and abort themselves if it's not the expected mode."
> When I know about the heap I do
>      Arguments::PropertyList_add(new SystemProperty("com.sap.vm.test.compressedOopsMode",
>                                                     narrow_oop_mode_to_string(narrow_oop_mode()),
>                                                     false));
> in universe.cpp.
> On some OSes it's deterministic which modes work, there we don't start such tests.
> Others, as you mentioned OSX,  are very indeterministic.  Here we save testruntime with this.
> But it's not that important.
> We can still parse the PrintCompresseOopsMode output after the test and discard the
> run.
>> Do you have platform specific changes?
> Yes, for ppc and aix.  I'll submit them once this is in.
>  From your other mail:
>> One more thing. You should allow an allocation in the range when returned from OS allocated address does not match
>> requested address. We had such cases on OSX, for example, when OS allocates at different address but still inside range.
> Good point.  I'll fix that in os::attempt_reserve_memory_in_range.
> I'll ping again once a new webrev is done!
> Best regards,
>    Goetz.
> On 11/10/14 6:57 AM, Lindenmaier, Goetz wrote:
>> Hi,
>> I need to improve a row of things around compressed oops heap handling
>> to achieve good performance on ppc.
>> I prepared a first webrev for review:
>> http://cr.openjdk.java.net/~goetz/webrevs/8064457-disjoint/webrev.00/
>> A detailed technical description of the change is in the webrev and according bug.
>> If requested, I will split the change into parts with more respective less impact on
>> non-ppc platforms.
>> The change is derived from well-tested code in our VM.  Originally it was
>> crafted to require the least changes of  VM coding, I changed it to be better
>> streamlined with the VM.
>> I tested this change to deliver heaps at about the same addresses as before.
>> Heap addresses mostly differ in lower bits.  In some cases (Solaris 5.11) a heap
>> in a better compressed oops mode is found, though.
>> I ran (and adapted) test/runtime/CompressedOops and gc/arguments/TestUseCompressedOops*.
>> Best regards,
>>     Goetz.

More information about the hotspot-dev mailing list