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

Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Dec 3 17:32:04 UTC 2014

Comments are below.

On 12/3/14 5:49 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir,
> thanks for looking at the change!  See my comments inline below.
> I made a new webrev:
> http://cr.openjdk.java.net/~goetz/webrevs/8064457-disjoint/webrev.02/
> Incremental changes:
> http://cr.openjdk.java.net/~goetz/webrevs/8064457-disjoint/webrev.02/incremental_diffs.patch
> Best regards,
>    Goetz.
>> -----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:
>> java.vm.compressedOopsMode=...
> 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?
> Fixed.
>> 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.
> Fixed.
>> In initialize_compressed_heap() again (!_base).
> Fixed.
>> 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.

I see what you are saying. It was not clear from comments what is going on.
Add more extending comment explaining that.

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

You need an other parameter to pass UnscaledOopHeapMax or zerobased_max.

> That makes the code not much more readable.  The function will look like this:

I think initialize_compressed_heap() is more readable now.

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

I mean that you already checked _base == NULL so on other side of || _base != NULL - why you need (_base &&) check?


> Thanks,
> Vladimir
> 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