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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Mon Dec 8 15:54:36 UTC 2014


Hi,

This is just a ping to gc/rt mailing lists to reach appropriate
people.

I please need a reviewer from gc or rt, could somebody have a 
look at this?

Short summary:
- new cOops mode disjointbase that allows optimizations on PPC improving over heapbased
- search for heaps: finds zerobased on sparc Solaris 11 and Aix
- concentrate cOops heap allocation code in one function
http://cr.openjdk.java.net/~goetz/webrevs/8064457-disjoint/webrev.02/

Please reply only to the original thread in hotspot-dev to keep this
local.

Thanks and best regards,
  Goetz.

-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
Sent: Donnerstag, 4. Dezember 2014 19:44
To: Lindenmaier, Goetz
Cc: 'hotspot-dev developers'
Subject: Re: RFR(L): 8064457: Introduce compressed oops mode "disjoint base" and improve compressed heap handling.

This looks good to me.

Now we need second review since changes are significant. Preferable from 
GC group since you changed ReservedHeapSpace. They will be affected 
most. Review from Runtime is also welcome.

Thanks,
Vladimir

On 12/4/14 10:27 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir.
>
> Sorry.  I updated the webrev once more.  Hope it's fine now.
> At least I can write comments :)
>
> Best regards,
>    Goetz
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Thursday, December 04, 2014 5:54 PM
> To: Lindenmaier, Goetz
> Cc: 'hotspot-dev developers'
> Subject: Re: RFR(L): 8064457: Introduce compressed oops mode "disjoint base" and improve compressed heap handling.
>
> I spotted an other bug.
> You replaced !_base with _base != NULL when moved code to try_reserve_range() - it should be _base == NULL.
> The same problem in asserts:
>
> +  assert(_base != NULL || markOopDesc::encode_pointer_as_mark(_base)->decode_pointer() == _base,
> +         "area must be distinguishable from marks for mark-sweep");
> +  assert(_base != NULL || markOopDesc::encode_pointer_as_mark(&_base[size])->decode_pointer() == &_base[size],
> +         "area must be distinguishable from marks for mark-sweep");
>
>
> Also you did not remove  _base && in next place:
>
> +         (_base && _base + size > zerobased_max))) {  // Unscaled delivered an arbitrary address.
>
> New comment is good.
>
> Thanks,
> Vladimri
>
> On 12/4/14 1:45 AM, Lindenmaier, Goetz wrote:
>> Hi Vladimir,
>>
>>> Add more extending comment explaining that.
>> The comment for try_reserve_heap was meant to explain that.
>> I further added a comment in initialize_compressed_heap().
>>
>>> You need another parameter to pass UnscaledOopHeapMax or zerobased_max.
>> Oh, thanks a lot!  That's important. Fixed.
>>
>>> I mean that you already checked _base == NULL so on other side of || _base != NULL - why you need (_base &&) check?
>> Sorry, now I got it.  Removed.
>>
>> I updated the webrev:
>> http://cr.openjdk.java.net/~goetz/webrevs/8064457-disjoint/webrev.02/
>> Increment on top of the increment :)
>> http://cr.openjdk.java.net/~goetz/webrevs/8064457-disjoint/webrev.02/incremental_diffs2.patch
>>
>> Thanks,
>>     Goetz.
>>
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Mittwoch, 3. Dezember 2014 18:32
>> 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.
>>
>> 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
>>
>>>
>>>
>>>
>>> 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-gc-dev mailing list