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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Mon Jan 5 09:24:41 UTC 2015


Hi Coleen, 

It would be great if you could sponsor this change.

> 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 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. 

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