RFR(L): 8064457: Introduce compressed oops mode "disjoint base" and improve compressed heap handling.
Christian Thalinger
christian.thalinger at oracle.com
Thu Dec 4 18:34:36 UTC 2014
> On Dec 4, 2014, at 10:27 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
>
> Hi Vladimir.
>
> Sorry. I updated the webrev once more. Hope it's fine now.
> At least I can write comments :)
Comments are more important than the code, right? ;-)
>
> 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-dev
mailing list