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

Coleen Phillimore coleen.phillimore at oracle.com
Tue Jan 6 18:28:13 UTC 2015


You're welcome.  I updated the files to 2015.  Happy New Year!
Coleen

On 1/6/15, 2:56 AM, Lindenmaier, Goetz wrote:
> Hi Coleen,
>
> thanks a lot for pushing this change!
>
> Best regards,
>    Goetz.
>
> -----Original Message-----
> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
> Sent: Monday, January 05, 2015 6:01 PM
> To: Lindenmaier, Goetz; hotspot-runtime-dev at openjdk.java.net; 'hotspot-dev at openjdk.java.net'
> Subject: Re: RFR(L): 8064457: Introduce compressed oops mode "disjoint base" and improve compressed heap handling.
>
>
> On 1/5/15, 11:49 AM, Coleen Phillimore wrote:
>> On 1/5/15, 4:24 AM, Lindenmaier, Goetz wrote:
>>> Hi Coleen,
>>>
>>> It would be great if you could sponsor this change.
>> I see no other comments so I'll push it today.
> I am planning to push this to hs-rt, btw.
> Coleen
>
>>>> The metaspace case probably can be fixed to use the
>>>> try_reserve_heap() code maybe.
>>> We wondered why the compressed class space is always above the heap.
>>> If the heap is too big for unscaled, the compressed class space could
>>> be placed
>>> in the lower 4G.
>> The reason that we chose the compressed class space on the top of the
>> heap is for a couple reasons.  The time spent compressing and
>> decompressing oops is more critical than the time spent doing the same
>> for the _klass pointer, so we wanted to make the system more likely to
>> get unscaled compressed oops, or even zero based compressed oops.
>>
>> On solaris, the space below the heap is limited because by default
>> malloc uses it so we were limited in low address memory that we can
>> use.  We want the compressed class space code not be platform
>> dependent.  We also need to have the CDS archive at a fixed address
>> near or adjacent to the compressed class space, so were running out at
>> the bottom of the heap.
>>
>> The compressed class space is fixed at startup (just like PermGen was)
>> and the classes are variable sized, so we had to pick a size fairly
>> large so that it's unlikely for applications to run out. If above 32G,
>> we might be able to implement code to allocate a new space which would
>> use the same compression algorithm.  We haven't had a request for that
>> but it's something we could do.
>>
>>
>>> The aarch port brings some changes here, and we would like to
>>> investigate
>>> this on top of the aarch changes, if that's fine with you.
>> So one thing I prototyped and really wanted to check in was what we
>> called indirect class pointers.  Instead of allocating classes in a
>> fixed class space, we allocate a fixed array of Klass pointers and the
>> objects in the heap point to this.   It had a 8% performance
>> degradation in specbb2008 compiler.compiler (iirc, or something like
>> that).
>>
>> This solved so many problems though.  If you investigated this sort of
>> approach, I'd be very grateful.   I haven't had time to go back to
>> figure out why this had this degradation (we thought it was Java
>> vtable accesses).
>>
>> Thanks,
>> Coleen
>>> Thanks a lot for going through this complex review!
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>> -----Original Message-----
>>> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
>>> Sent: Freitag, 2. Januar 2015 17:27
>>> To: Lindenmaier, Goetz; hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: RFR(L): 8064457: Introduce compressed oops mode
>>> "disjoint base" and improve compressed heap handling.
>>>
>>>
>>> On 1/2/15, 10:09 AM, Lindenmaier, Goetz wrote:
>>>> Hi Coleen,
>>>>
>>>> I looked at the cleanups and found both not straight forward to
>>>> implement.
>>>>
>>>> 1) Merging ReservedHeap::initialize and
>>>> ReservedHeapSpace::try_reserve_heap.
>>>>     The new code calls initialize() with requested_address = NULL,
>>>> but metaspace.cpp
>>>> passes a real requested_address.  Therefore I can not clean up the
>>>> calls to
>>>> failed_to_reserve_as_requested() in initialize().  Without that
>>>> cleanup, I need to pass flags
>>>> into initialize() to configure all the different behaviours I would
>>>> need in a
>>>> merged case.
>>>> I think with 'large' and 'requested_address' there is already enough
>>>> cases
>>>> in that method, so that I don't want to add more.
>>> Okay, this is fine.  More flags to special case functions aren't good
>>> either.  Can you file an RFE to look into refactoring so that the
>>> duplicate code can be removed though?   The metaspace case probably can
>>> be fixed to use the try_reserve_heap() code maybe.
>>>
>>>> 2) Moving _noaccess_prefix to ReservedHeapSpace
>>>>
>>>> This does not work as  ReservedHeapSpace is casted to
>>>> ReservedSpace and returned by value.  So ReservedHeapSpace can not have
>>>> fields or virtual methods.
>>>> It would be probably better to change the code to return
>>>> ReservedHeapSpace
>>>> by pointer, or, at least under that type, but I think that exceeds
>>>> the scope of this change.
>>> Too bad.
>>>
>>>> So is it ok if I leave the change as is wrt. to these cleanups?
>>> Yes, this is fine.
>>>> I extended test/runtime/CompressedOops/UseCompressedOops.java
>>>> to combine the testcases of the different mode with the GCs and
>>>> UseLargePages.
>>>> I added the changes in the test to
>>>> http://cr.openjdk.java.net/~goetz/webrevs/8064457-disjoint/webrev.03/
>>> Is .03 with the additional changes?  If so, you should have made an .04.
>>>
>>> I reviewed this again and it looks good.   I think Vladimir has reviewed
>>> an earlier version.  I can sponsor this assuming there are no more
>>> comments.
>>>
>>> Coleen
>>>> Best regards,
>>>>      Goetz.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
>>>> Sent: Dienstag, 30. Dezember 2014 16:49
>>>> To: Lindenmaier, Goetz; hotspot-runtime-dev at openjdk.java.net
>>>> Subject: Re: RFR(L): 8064457: Introduce compressed oops mode
>>>> "disjoint base" and improve compressed heap handling.
>>>>
>>>>
>>>> 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