RFR(M): 8204908: NVDIMM for POGC and G1GC - ReserveSpace.cpp changes are mostly eliminated/no collector specific code.

sangheon.kim at oracle.com sangheon.kim at oracle.com
Mon Sep 17 18:31:58 UTC 2018


Hi Kishor,


On 9/4/18 10:41 PM, Kharbas, Kishor wrote:
> Hi,
> I have uploaded implementation for parallel scavenge at http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.00
> Majority of the implementation is handled in two new files - adjoiningGenerationsForHeteroHeap.cpp and psFileBackedVirtualspace.cpp. Would love to hear your feedback and suggestions.
Sorry for late review.

----------------
General comments.

1. Looks like this patch is not based on latest repository, as it fails 
with -Wreorder issue.

2. I see many wrong alignment location of having only 2 spaces after new 
line that is continued.
e.g.
a->b(c,
__d, xxxx); // 2 spaces
this should be
a->b(c,
_____d, xxx); // 'd' should locate under 'c' as those are in the same 
context.

3. I see assertion failures with below options combinations. There could 
be more... Please run jtreg tests before posting webrev.
-XX:+UseLargePages -XX:+UseSHM -version
-XX:+UseLargePages -XX:+UseNUMA -version
-XX:+UseLargePages -XX:AllocateHeapAt=. -version

=========================
src/hotspot/share/gc/parallel/adjoiningGenerations.cpp
- Copyright update

43   _virtual_spaces(new AdjoiningVirtualSpaces(old_young_rs, 
policy->min_old_size(),
44                   policy->min_young_size(), alignment) ) {
- Wrong alignment?


=========================
src/hotspot/share/gc/parallel/adjoiningGenerations.hpp
- Copyright update

62   AdjoiningGenerations();
- Why we need this ctor?


=========================
/src/hotspot/share/gc/parallel/adjoiningVirtualSpaces.hpp
- Copyright update

   88   virtual PSVirtualSpace* high() { return _high; }
   89   virtual PSVirtualSpace* low()  { return _low; }
- Those methods don't need 'virtual' specifier as high()/low() methods 
are only used at ctor of AdjoiningGenerations. The other calling site is 
ctor of AdjoiningGenerationsForHeteroHeap but it is used another type of 
high()/low() which returns _young_vs or _old_vs.


=========================
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp
- no comments.


=========================
src/hotspot/share/gc/parallel/psOldGen.cpp
=========================
src/hotspot/share/gc/parallel/psOldGen.hpp
   32 #include "gc/parallel/psFileBackedVirtualspace.hpp"
- Include this header file at cpp seems better rather than hpp.

=========================
src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.cpp
    1 /*
    2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights 
reserved.
    3 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
- Wrong alignment. The second/below star should locate under first line 
star.
- Similarily there are more wrong alignment locations.
   . Line 49, 50
   . 52, 53
   . 54, 55
   . 64, 65
   . 67, 68 69 70
   . 72, 73 74 75 76
   . 79, 80
   . 81, 82
   . 85, 86
   . 87, 88
   . 106, 107 108 109
   . 114, 115
   . 166, 167 168
   . 178, 179 180
   . 186, 187 188
   . 218, 219 220
   . 230, 231 232
   . 239, 240 241


   59   // Initialize the virtual spaces.  Then pass the
   60   // a virtual to each generation for initialization of the
- Then pass 'the a' virtual to each generation. 'the a'?

   64   (static_cast 
<HeteroVirtualSpaces*>(_virtual_spaces))->initialize(max_old_byte_size, 
init_old_byte_size,
   65     init_young_byte_size);
- Just making 'initilize' method as 'virtual' seems better.

   39 
AdjoiningGenerationsForHeteroHeap::AdjoiningGenerationsForHeteroHeap(ReservedSpace 
old_young_rs, size_t total_size_limit,
   40   GenerationSizer* policy, size_t alignment) : 
_total_size_limit(total_size_limit) {
- Wrong alirnment at line 40. 'Generation' should be under 
'ReservedSpace' at line 39.

   49   _virtual_spaces = new HeteroVirtualSpaces(old_young_rs, 
min_old_byte_size,  70     (static_cast 
<HeteroVirtualSpaces*>(_virtual_spaces))->max_young_size());
   75     (static_cast 
<HeteroVirtualSpaces*>(_virtual_spaces))->max_old_size(),
- Instead assigning at line 49 and then cast to HeteroVirtualSpaces*, 
create/initialize HeteroVirtualSpaces, get max_young/old_size() and the 
assign to _virtual_spaces would be better alternative.

  109   _min_young_byte_size(min_yg_byte_size), 
_max_total_size(max_total_size) {
  110   _max_old_byte_size = _max_total_size - _min_young_byte_size;
  111   _max_young_byte_size = _max_total_size - _min_old_byte_size;
- _max_old_byte_size and _max_young_byte_size can move to initialization 
list.

  117   // This the reserved space exclusively for old generation.
  122   // This the reserved space exclusively for young generation.
- Missing 'is'? i.e. // This *is* the reserved space exclusively for old 
generation.

  131     vm_exit_during_initialization("Could not reserve enough space 
for "
  132       "object heap");
- 'object heap' can stay same line. Or changing align is necessary.

  137     vm_exit_during_initialization("Could not reserve enough space 
for "
  138       "object heap");
- 'object heap' can stay same line. Or changing align is necessary.

  152   if (uncommitted_in_old > 0) {
- This condition seems redundant as uncommitted_in_old is type of size_t.

  159   if (bytes_needed == 0) {
  160     return true;
  161   }
- This condition, can move to inside of 'if (uncommitted_in_old > 0)' 
condition because if uncommitted_in_old is zero, there's no way 
bytes_needed to be zero - bytes_needed is guaranteed not to be zero from 
caller site, so safe to move between line 154 and 155. The condition to 
return true is 'uncommitted_size == bytes_needed && success of expand_by()'.

  176     bool ret = _young_vs->shrink_by(shrink_size);
  177     assert(ret, "We should be able to shrink young space");
- I would prefer to add more conditions below if we fails to shrink. 
i.e. just assert seems not enough.

  189   _old_vs->expand_by(bytes_to_add_in_old);
- Check the return value of expand_by().

  211   if (bytes_needed == 0) {
  212     return true;
  213   }
- Same as 'adjust_boundary_down()'

  244   DEBUG_ONLY(size_t total_size_after = _young_vs->reserved_size() 
+ _old_vs->reserved_size());
  245   assert(total_size_after == total_size_before, "should be equal");
- Why adjust_boundary_up() doesn't have this kind of check?


=========================
src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.hpp
    1 /*
    2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights 
reserved.
    3 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
    4 *
- Wrong alignment. The second/below star should locate under first line 
star.

   37   size_t total_size_limit() {
   38     return _total_size_limit;
   39   }
- I don't think we need this but if you prefer to have, add 'const'.

   45     PSVirtualSpace*    _young_vs;
   46     PSVirtualSpace*    _old_vs;
- Can't we use 'AdjoiningVirtualSpaces::_low' and '_high' instead? If it 
is not the case, adding high(),low() may result in confusion between 
AdjoiningVirtualSpaces::high() and low(). So use other name for current 
HeteroVirtualSpaces::high()/low().

   55     HeteroVirtualSpaces(ReservedSpace rs,
   56       size_t min_old_byte_size,
   57       size_t min_young_byte_size, size_t max_total_size,
   58       size_t alignment);
- Wrong alignment. Line 56 ~ 58 should be same as the location of 
'ReservedSpace' at line 55.

   67     size_t max_young_size() { return _max_young_byte_size; }
   68     size_t max_old_size() { return _max_old_byte_size; }
- 'const'?

   70     void initialize(size_t initial_old_reserved_size, size_t 
init_low_byte_size,
   71       size_t init_high_byte_size);
- Wrong alignment

   82   size_t reserved_byte_size();
- 'const'?

=========================
  src/hotspot/share/gc/parallel/psFileBackedVirtualspace.cpp
    1 /*
    2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights 
reserved.
- Wrong alignment. The second/below star should locate under first line 
star.

   36   if (_fd == -1) {
   37     return;
   38   }
- I prefer to have 'initialize()' method to handle when fails to create 
the heap file. Current implementation seems easy to call additional 
'initialize()'.
   Ctor of PSVirtualSpace doesn't have any failure path(e.g. 
allocation), so no further check is needed around line 18:psOldGen.cpp. 
But yours is different, so something should be checked.

   33   _mapping_succeeded = false;
   34   _file_path = path;
   46   _mapping_succeeded = true;
   47   _special = true;
- Can move to initialization list with proper initial value.

   55   assert(special(), "_special should always be true for 
PSFileBackedVirtualSpace");
   66   assert(special(), "_special should always be true for 
PSFileBackedVirtualSpace");
- For these 2, can we have more specific message? e.g. to include 
meaning of expand or shrink.


=========================
  src/hotspot/share/gc/parallel/psFileBackedVirtualspace.hpp
    1 /*
    2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights 
reserved.
- Wrong alignment. The second/below star should locate under first line 
star.

   37   PSFileBackedVirtualSpace(ReservedSpace rs, const char* file_path);
   38   bool   expand_by(size_t bytes);
- Just recommendation to add new line between 37 and 38.

   43 #endif // SHARE_VM_GC_PARALLEL_PSFILEBACKEDVIRTUALSPACE_HPP
- Last line is not terminated with a newline.


Thanks,
Sangheon


>
> I will post G1 GC implementation in a few days.
>
> Thanks
> Kishor.
>
>> -----Original Message-----
>> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
>> Sent: Thursday, August 30, 2018 4:06 PM
>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Thomas Stüfe
>> <thomas.stuefe at gmail.com>; Thomas Schatzl
>> <thomas.schatzl at oracle.com>
>> Cc: hotspot-gc-dev at openjdk.java.net; Hotspot dev runtime <hotspot-
>> runtime-dev at openjdk.java.net>; Viswanathan, Sandhya
>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>> <shirish.aundhe at intel.com>
>> Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>> ReserveSpace.cpp changes are mostly eliminated/no collector specific code.
>>
>> Hi Kishor,
>>
>> On 8/30/18 11:55 AM, Kharbas, Kishor wrote:
>>> Hi Sangheon,
>>>
>>> So far I don’t see a need to do so. I will post my implementation code
>> today or tomorrow, if we see a need or any simplification by changing
>> classes in VirtualSpace.hpp, we can discuss that.
>> Okay.
>>
>> Thanks,
>> Sangheon
>>
>>
>>> Regards,
>>> -Kishor
>>>
>>>> -----Original Message-----
>>>> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
>>>> Sent: Wednesday, August 29, 2018 10:17 AM
>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Thomas Stüfe
>>>> <thomas.stuefe at gmail.com>; Thomas Schatzl
>> <thomas.schatzl at oracle.com>
>>>> Cc: hotspot-gc-dev at openjdk.java.net; Hotspot dev runtime <hotspot-
>>>> runtime-dev at openjdk.java.net>; Viswanathan, Sandhya
>>>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>>>> <shirish.aundhe at intel.com>
>>>> Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>>>> ReserveSpace.cpp changes are mostly eliminated/no collector specific
>> code.
>>>> Hi Kishor,
>>>>
>>>> On 8/29/18 9:52 AM, Kharbas, Kishor wrote:
>>>>> Hi Sangheon,
>>>>>
>>>>> Thanks for reviewing the design.
>>>>> Since the collectors do not use them for heap memory, I did not have
>>>>> to override VirtualSpace
>>>> Sorry, I meant ReservedSpace and its friends at
>>>> share/memory/virtualspace.hpp.
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>> -Kishor
>>>>>> -----Original Message-----
>>>>>> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
>>>>>> Sent: Tuesday, August 28, 2018 2:38 PM
>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Thomas Stüfe
>>>>>> <thomas.stuefe at gmail.com>; Thomas Schatzl
>>>> <thomas.schatzl at oracle.com>
>>>>>> Cc: hotspot-gc-dev at openjdk.java.net; Hotspot dev runtime <hotspot-
>>>>>> runtime-dev at openjdk.java.net>; Viswanathan, Sandhya
>>>>>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>>>>>> <shirish.aundhe at intel.com>
>>>>>> Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>>>>>> ReserveSpace.cpp changes are mostly eliminated/no collector
>>>>>> specific
>>>> code.
>>>>>> Hi Kishor,
>>>>>>
>>>>>> On 8/21/18 10:57 AM, Kharbas, Kishor wrote:
>>>>>>> Hi All,
>>>>>>>
>>>>>>> Thank you for your valuable comments and feedback in this thread
>>>>>>> so
>>>> far.
>>>>>>> After taken in all the comments and reading thoroughly through the
>>>>>>> code, I
>>>>>> feel that the complexity added to virtualSpace.cpp is due to lack
>>>>>> of abstraction in VirtualSpace and at GC level. NV-DIMM size and
>>>>>> file paths are being passed all the way to OS calls.
>>>>>>> So I went back to the drawing board and created a high level
>>>>>>> design to remove all the pain points of current implementation. I
>>>>>>> have uploaded the design at
>>>>>>>
>> http://cr.openjdk.java.net/~kkharbas/8202286/Design%20for%20JEP%20JDK
>>>>>> -
>>>>>>> 8202286.pdf I would love to hear your feedback and suggestions.
>>>>>>> Once we get confidence in the design, I will work on the
>> implementation.
>>>>>> The design looks good to me.
>>>>>> If you are planning to change VirtualSpace at
>>>>>> share/memory/virtualspace.hpp, including it on the design document
>>>>>> would be helpful too.
>>>>>>
>>>>>> Probably folks involved in the previous discussions would say
>>>>>> whether the design well covers their concerns or not.
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>>
>>>>>>
>>>>>>> PS:
>>>>>>> 1. Vinay has transitioned to another team in Intel, so he won't be
>>>>>>> able to
>>>>>> continue on this jep.
>>>>>>> 2. If you feel this should be part of JEP discussion thread, we
>>>>>>> can take it
>>>>>> there.
>>>>>>> Thanks and regards,
>>>>>>> Kishor
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
>>>>>>>> Sent: Friday, June 22, 2018 9:25 AM
>>>>>>>> To: Thomas Schatzl <thomas.schatzl at oracle.com>
>>>>>>>> Cc: Awasthi, Vinay K <vinay.k.awasthi at intel.com>; Paul Su
>>>>>>>> <paul.su at oracle.com>; hotspot-gc-dev at openjdk.java.net; Hotspot
>>>> dev
>>>>>>>> runtime <hotspot-runtime-dev at openjdk.java.net>; Viswanathan,
>>>>>> Sandhya
>>>>>>>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>>>>>>>> <shirish.aundhe at intel.com>; Kharbas, Kishor
>>>>>>>> <kishor.kharbas at intel.com>
>>>>>>>> Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>>>>>>>> ReserveSpace.cpp changes are mostly eliminated/no collector
>>>>>>>> specific
>>>>>> code.
>>>>>>>> Hi Thomas,
>>>>>>>>
>>>>>>>> On Fri, Jun 22, 2018 at 4:44 PM, Thomas Schatzl
>>>>>>>> <thomas.schatzl at oracle.com> wrote:
>>>>>>>>> Hi Thomas,
>>>>>>>>>
>>>>>>>>> On Tue, 2018-06-19 at 13:40 +0200, Thomas Stüfe wrote:
>>>>>>>>>> Hi Vinay,
>>>>>>>>>>
>>>>>>>>>> On Mon, Jun 18, 2018 at 6:47 PM, Awasthi, Vinay K
>>>>>>>>>> <vinay.k.awasthi at intel.com> wrote:
>>>>>>>>>>> Hi Thomas,
>>>>>>>>>>>
>>>>>>>>>>> Os::commit_memory calls map_memory_to_file which is same
>> as
>>>>>>>>>>> os::reserve_memory.
>>>>>>>>>>>
>>>>>>>>>>> I am failing to see why os::reserve_memory can call
>>>>>>>>>>> map_memory_to_file (i.e. tie it to mmap) but commit_memory
>>>>>> can't...
>>>>>>>>>>> Before this patch, commit_memory never dealt with
>>>>>>>>>>> incrementally committing pages to device so there has to be a
>>>>>>>>>>> way to pass file descriptor and offset. Windows has no such
>>>>>>>>>>> capability to manage incremental commits. All other OSes do
>>>>>>>>>>> and that is why map_memory_to_file is used (which by the way
>>>>>>>>>>> also works on Windows).
>>>>>>>>>> AIX uses System V shared memory by default, which follows a
>>>>>>>>>> different allocation scheme (semantics more like Windows
>>>>>> VirtualAlloc...
>>>>>>>>>> calls).
>>>>>>>>>>
>>>>>>>>>> But my doubts are not limited to that one, see my earlier
>>>>>>>>>> replies and those of others. It really makes sense to step back
>>>>>>>>>> one step and discuss the JEP first.
>>>>>>>>>>
>>>>>>>>> There is already a discussion thread as David mentioned
>>>>>>>>> (http://mail.op
>>>>>>>>> enjdk.java.net/pipermail/hotspot-gc-dev/2018-May/022092.html)
>>>> that
>>>>>>>>> so far nobody answered to.
>>>>>>>>>
>>>>>>>> Ah, I thought he wanted to have the JEP discussed in the comments
>>>>>>>> section of the JEP itself.
>>>>>>>>
>>>>>>>>> I believe discussion about contents the JEP and the
>>>>>>>>> implementation should be separate.
>>>>>>>>>
>>>>>>>> makes sense.
>>>>>>>>
>>>>>>>>> So far what I gathered from the responses to the implementation,
>>>>>>>>> the proposed idea itself is not the issue here (allowing the use
>>>>>>>>> of NVDIMM memory for parts of the heap for allowing the use of
>>>>>>>>> larger heaps to improve overall performance; I am not saying
>>>>>>>>> that the text doesn't need a bit more work :) ), but rather how
>>>>>>>>> an implementation of this JEP should proceed.
>>>>>>>> I have no problem with adding NVDIMM support. I think it is a
>>>>>>>> cool
>>>>>> feature.
>>>>>>>> Also, the awkwardness off the memory management abstraction
>> layer
>>>>>>>> in hotspot has always been a sore point with me (I originally
>>>>>>>> implemented the AIX mm layer in os_aix.cpp, and those are painful
>>>>>>>> memories). So, I have a lot of sympathy for Vinays struggles.
>>>>>>>> Unfortunately not much time atm, but I will respond to your mail.
>>>>>>>>
>>>>>>>>> Let's discuss the non-implementation stuff in that thread. Vinay
>>>>>>>>> or I can repost the proposal email if you do not have it any
>>>>>>>>> more so that answers will be in-thread.
>>>>>>>>>
>>>>>>>> Okay, sounds good.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>       Thomas
>>>>>>>>
>>>>>>>> (one of us should change his name to make this less confusing :-)
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>       Thomas
>>>>>>>>>



More information about the hotspot-gc-dev mailing list