RFR(L): 8064457: Introduce compressed oops mode "disjoint base" and improve compressed heap handling.
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Dec 30 15:48:54 UTC 2014
On 12/30/14, 8:42 AM, Lindenmaier, Goetz wrote:
> Hi Coleen,
>
>> disjoint
> It tries to say that the bits used for the base (36-63) are different from
> those used for the oops / used as offset into the heap (0-35).
> I chose the name according to this translation
> http://dict.leo.org/#/search=disjoint&searchLoc=0&resultOrder=basic&multiwordShowSingle=on
> Both the german adjective and verb fit to describe the property of the base.
> I can add the following to the docu of the mode in universe.hpp:358
> // Disjoint: Bits used in base are disjoint from bits used
> // for oops ==> oop = (cOop << 3) | base. One can disjoint
> // the bits of an oop into base and compressed oop.
Yes, that helps. It's a good distinct word so its good that we can make
sense in the documentation.
>> (I filed a bug to move these to the memory directory)
> Good idea!
>
>> 272 void ReservedSpace::establish_noaccess_prefix() {
>> This function should be in ReservedHeapSpace
> Yes, I moved it. I also would like to move the field _noaccess_prefix,
> but I would have to change a lot of code, like ReservedSpace::release().
> The problem is that _base is not the real base in ReservedHeapSpace.
> Probably ReservedHeapSpace should have heap_size() and heap_base()
> giving the correcte values.
> I'll try to come up with a patch on top of this, maybe we should do it
> in a separate bug.
I just moved _noaccess_prefix and added a virtual
ReservedHeapSpace::release() that adjusted the base and size with
noaccess_prefix. I don't think the footprint of a vtable or virtual
call makes any difference.
>
>> Can you have arguments.cpp verify_range of HeapSearchSteps so this can't
>> happen?
> Done.
>
>> Rather move this to the end of ReservedHeapSpace constructor.
>> assert(_base == NULL || ...
> Yes, you are right, it should not be there twice. I moved it into the
> constructor. It is only relevant for heaps, right?
Yes.
>
>> Code in try_reserve_heap() is mostly a copy of ReservedSpace::initialize()
> You are right, requested_address in initialize is dead now. I'll remove that
> and try to merge the two functions.
>
> I removed the verbose printing. Anyways, it's mostly useful when developing this.
Thanks. I was trying to find some way to move the printing so it would
make sense to me but ended up giving up on that.
>> aligned_HBMA
> Changed to aligned_heap_base_min_address.
>
>> Is "size" const in initialize_compressed_heap?
> Yes. Added 'const'.
>
> I'll run tests with +UseLargePages.
>
> I had verified that the heap is at the exact same position for the code in
> my first RFR, and with HeapSearchSteps=1 to make sure I don't break anything.
> Vladimir asked me to increase that to 3. So far I still didn't find a case where
> this changed anything.
> (But I know from SAP JVM, which is tested on all the OS variants we support,
> that there are cases where this can lead to a better - but then different - heap
> placement.)
> The only case where there is always a difference is Solaris 5.11 which
> now can allocate zerobased memory.
> The code passes the CDS jtreg tests.
>
> I updated the webrev.03 with the minor changes.
> I'll come up with a new one with the cleanups of Reserved(Heap)Space
> (_noaccess_prefix and redundant functions).
>
> Thanks for this thorough review!!
Thank you for your willingness to make these changes.
Happy New Year!
Coleen
>
> Happy new year,
> Goetz.
>
>
>
>
> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
> Sent: Dienstag, 30. Dezember 2014 03:06
> 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, I've reviewed more code.
>
> I'm sorry about the following comment that should have been asked
> before. The name of the mode is very confusing. I can't figure out why
> it's called "disjoint". I keep looking for a second heap base or some
> code that makes the heap discontinuous. The heap base is really
> specially aligned, right? So I went to the thesaurus.com:
>
> http://www.thesaurus.com/browse/aligned
>
> and "disjoin" is an antonym. Why was "disjoint" chosen? From the
> code, it looks like "aligned" or "scaled" are more descriptive but
> they're overused words in the code. But ScaledBasedNarrowOop seems more
> descriptive than Disjoint.
>
> In virtualspace.cpp (I filed a bug to move these to the memory directory)
>
> 272 void ReservedSpace::establish_noaccess_prefix() {
>
>
> This function should be in ReservedHeapSpace since ReservedSpace never
> does this (and we never want it to). The function it replaced was in
> ReservedSpace and that was wrong. Now that ReservedHeapSpace has
> compressed oops logic in it, I think this function should be moved to
> this class. Actually, I've modified your patch to move the
> _noaccess_prefix field to ReservedHeapSpace. Maybe that's too much
> change to your patch but since the handling of noaccess_prefix is
> changed, it seemed like the thing to do.
>
> Can you have arguments.cpp verify_range of HeapSearchSteps so this can't
> happen?
>
> guarantee(HeapSearchSteps > 0, "Don't set HeapSearchSteps to 0");
>
> Can this be a function like check_encoding_for_mark_sweep() , or
> something like that? It appears twice. Rather move this to the end of
> ReservedHeapSpace constructor.
>
> 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");
>
> Code in try_reserve_heap() is a mostly a copy of
> ReservedSpace::initialize(), except the first version has a call to
> failed_to_reserve_as_requested which looks like it's handling the case
> for not being able to reserve the requested address for compressed oops
> which seems like you don't want this. This is very confusing. There
> shouldn't be two of these.
>
> It looks like in the compressed oops case, you call
> ReservedSpace::initialize() only with NULL requested address so
> failed_to_reserve_as_requested is dead code with this change. So it
> seems like try_reserve_heap() could be ReservedSpace::initialize() with
> the failed_to_reserve_as_requested eliminated and a special case for
> handling unaligned addresses.
>
> The logic in initialize_compressed_heap() still doesn't make sense to
> me. If the first try_reserve_range() succeeds at getting an unscaled
> compressed oop base, then does PrintCompressedOopsMode && Verbose still
> print:
>
> tty->print(" == U N S C A L E D ==\n");
> tty->print(" == Z E R O B A S E D ==\n");
> tty->print(" == D I S J O I N T B A S E ==\n");
> tty->print(" == H E A P B A S E D ==\n");
>
> Why would you print all 4? It's the printing that makes this
> confusing! Now I see how the next case checks that the base address is
> a better allocation. If you changed the printing to a comment like:
>
> // UnscaledMode after testing previous base address is not better (or
> something).
>
> Actually, you have these comments. I think the printing should be
> removed because it's the opposite of helpful.
>
> I had trouble reading the parameter aligned_HBMA, can you change to
> aligned_heap_base_min (I know it's longer but there's too many
> calculations here that words will make easier to read). Or just
> heap_base_min_address.
>
> zerobased_max = (char *)OopEncodingHeapMax - class_space;
>
> Odd that the solaris compiler can compile this and not the code below.
> (minor point) I think this should be:
>
> zerobased_max = zerobased_max - class_space;
>
> Is "size" const in initialize_compressed_heap? (answer appears to be
> yes) It's useful for reading the calculations that it never changes so
> I think it should be const (it's a minor point too).
>
> I don't have any other comments, I've tried to do the calculations a bit
> but I know that you've done this yourself.
>
> Also, can you run tests with -XX:+UseLargePages? I assume you've run
> tests with the 4 garbage collectors. This is a tricky piece of code
> that replaced something else difficult. We've had to change it often to
> support other features like CDS and compressed class pointers so making
> it as understandable as possible is worth the extra effort. I think
> your new design looks cleaner.
>
> Thanks,
> Coleen
>
>
> On 12/24/14, 6:13 PM, Coleen Phillimore wrote:
>> One note for my first reviewed file.
>>
>> On 12/22/14, 5:30 PM, Coleen Phillimore wrote:
>>> Hi Goetz,
>>>
>>> I'm looking at this version of the code
>>> http://cr.openjdk.java.net/~goetz/webrevs/8064457-disjoint/webrev.03/
>>>
>>> This passes our JPRT tests. Yeah!
>>>
>>> It's a significant change, so I'm actually reading the code with the
>>> patches applied.
>>>
>>> In src/share/vm/memory/universe.cpp
>>>
>>> Can you put all this code from 742-776 in a new function above
>>> initialize_heap(), like set_narrow_oop_values()?
>>> The comment at 689-695 refer to code that's moved to
>>> virtualspace.cpp. I think it needs to be reworded, updated for
>>> disjoint oops mode or removed. The comment at 744-749 also refers to
>>> code that's moved so should be removed also. The last two lines of
>>> this comment refer to something whose implementation has changed, so
>>> just remove them.
>>>
>>> On line
>>>
>>> 750 if ((uint64_t)Universe::heap()->reserved_region().end() >
>>> UnscaledOopHeapMax) {
>>>
>>> Can you make this expression a variable because the same expression
>>> appears on line 754.
>>>
>>> Why can't this code at 742-776 be with the compressed oops mode code
>>> at line 836-839. If you make this set_narrow_oop_values() function,
>>> can it be moved together?
>> Apparently this doesn't work (it crashes on some platforms). I can't
>> completely follow the control flow that makes this wrong, but ignore
>> this suggestion.
>>
>> Coleen
>>> Can you remove this comment too? I have no idea what it means. There
>>> is no CATCH below (?)
>>>
>>> // We will never reach the CATCH below since Exceptions::_throw
>>> will cause
>>> // the VM to exit if an exception is thrown during initialization
>>>
>>>
>>> ** We should file an RFE to move virtualspace.hpp/cpp to the
>>> src/share/vm/memory directory!
>>>
>>> Sorry, this is partial, I'll continue tomorrow.
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>> On 12/11/2014 06:18 AM, Lindenmaier, Goetz wrote:
>>>> 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