RFR(L): 8064457: Introduce compressed oops mode "disjoint base" and improve compressed heap handling.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Thu Dec 11 11:18:32 UTC 2014
Hi Coleen,
thanks for looking at this change!
> get_attach_addresses_for_disjoint_mode function belongs in virtualspace.cpp
All information about the bounds of the modes comes from universe. This is
related to is_disjoint_heap_base_address() and thus I left it in universe.
But I'll move it, it can also be regarded as an implementation detail of
initialize_compressed_heap(). I can make it static then, so that it's removed
in 32 bit build, which is good.
> Can you reduce some to at least 100?
Done. I agree that's better . (I have a 2560x1440 screen, causing me to have
big windows :))
> Why is highest_start not const char* ?
Removed the const.
> The control flow in initialize_compressed_heap makes no sense to me.
Vladimir had the same problem, I added comment at line 493 to make
that clearer.
One problem of the previous implementation was that, if the os layer
returned an address with desired properties, but not at the requested
address, it was discarded.
To fix this, try_reserve_heap always returns what it allocated, and I
check afterwards.
Therefore the checks of the next mode include the check of the
previous one. So if I try to allocate disjoint, but get unscaled,
I will detect that and return the unscaled heap.
If I implement a return, I just have to add tests. I don't get rid of
other tests.
> Also, our 32 bit platforms don't like: 1 * SIZE_64K * SIZE_32G,
I changed that to use UCONST64 macro. That should help I guess.
I also have these in globalDefinitions_xlc.hpp. Should I move them
to globalDefinitions.hpp:200, after definition of K, M and G?
I uploaded a new webrev:
http://cr.openjdk.java.net/~goetz/webrevs/8064457-disjoint/webrev.03/
Best regards,
Goetz.
-----Original Message-----
From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
Sent: Mittwoch, 10. Dezember 2014 20:22
To: hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(L): 8064457: Introduce compressed oops mode "disjoint base" and improve compressed heap handling.
Hi Goetz,
I have some initial comments, which are much less detailed than
Vladimir's comments. I haven't actually processed all the
implementation details yet.
I think get_attach_addresses_for_disjoint_mode function belongs in
virtualspace.cpp and not in universe. I like that the compressed oop
reservation logic is moved to virtualspace.cpp. I think this is an
improvement over the Universe::preferred_heap_base() logic which was
also difficult.
The Hotspot coding style doesn't dictate 80 columns anymore (there is
debate whether it should have or not, which we try not to have this
debate), but I found the very long lines in this code inconsistent with
other Hotspot code. I had to expand my window to cover my whole
screen. Can you reduce some to at least 100?
For example, I aligned these parameters to see better since there are so
many (the indentation isn't right in email).
void ReservedHeapSpace::try_reserve_range(char *const highest_start,
char *lowest_start,
size_t attach_point_alignment,
char *aligned_HBMA, //
HeapBaseMinAddress
char *upper_bound,
size_t size,
size_t alignment,
bool large) {
Why is highest_start not const char* ? Doesn't char* const
highest_start just restrict you from changing the pointer and not what
it points to? This seems odd to do.
The control flow in initialize_compressed_heap makes no sense to me.
It seems like there should be an exit when the best allocation for
compression is achieved. But it falls through after
try_reserve_range(). I can't figure out why it should fall through....
I was expecting something like:
if (PrintCompressedOopsMode && Verbose) {
tty->print(" == H E A P B A S E M I N A D D R E S S ==\n");
}
get the heap at aligned HeapBaseMinAddress, return if success...
if (PrintCompressedOopsMode && Verbose) {
tty->print(" == U N S C A L E D ==\n");
}
try to get heap base for unscaled access, return if successful
etc.
You should make this into a little function for each return and for the
end of the initialize function, or move it to the ReservedHeapSpace
constructor (caller).
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");
}
I ran this through JPRT and this line didn't compile on macos:
const uint64_t num_attempts_to_try = MIN2(HeapSearchSteps,
num_attempts_possible);
I'm retrying now as:
const uint64_t num_attempts_to_try =
MIN2((uint64_t)HeapSearchSteps, num_attempts_possible);
Sorry for the delay looking at this. This is a big change that I needed
to make more time to read. I am pleased that it's a performance
improvement.
Thanks,
Coleen
On 12/8/14, 10:54 AM, Lindenmaier, Goetz wrote:
> 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-runtime-dev
mailing list