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

Kharbas, Kishor kishor.kharbas at intel.com
Thu Oct 4 01:24:39 UTC 2018


Hi,
I have an update to the webrev which addresses comments from Sangheon during an offline discussion.

The JEP has been moved to an enhancement, consequently bug id '8204908' has been closed. I have created a new issue for this implementation - https://bugs.openjdk.java.net/browse/JDK-8211424.
Check the comments in main issue - https://bugs.openjdk.java.net/browse/JDK-8202286.
Any suggestion on whether I continue on this thread or start new one. Not only the bug id has changed, but also the design, implementation.

Full: http://cr.openjdk.java.net/~kkharbas/8211424/webrev-8204908.03
Incremental : http://cr.openjdk.java.net/~kkharbas/8211424/webrev-8204908.03_to_02

Testing : Passed tier1_gc and tier1_runtime jtret tests.
                 I added extra options "-XX:+UnlockExperimentalVMOptions -XX:AllocateOldGenAt=/home/Kishor" to each test. There are some tests which I had to exclude since they won't work with this flag. Example: tests in ConcMarkSweepGC which does not support this flag, tests requiring CompressedOops to be enabled, etc.

Thanks,
Kishor

> -----Original Message-----
> From: Kharbas, Kishor
> Sent: Wednesday, September 19, 2018 3:57 PM
> To: sangheon.kim at oracle.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>; 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,
> I have an small update to the patch to disable UseCompressedOops.
> http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.02/
> http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.02_to_01/
> 
> Regards,
> Kishor
> 
> > -----Original Message-----
> > From: Kharbas, Kishor
> > Sent: Wednesday, September 19, 2018 9:35 AM
> > To: 'sangheon.kim at oracle.com' <sangheon.kim at oracle.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>; 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.
> >
> > Thanks Sangheon,
> >
> > I have uploaded the updated patch at,
> > http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.01/
> > http://cr.openjdk.java.net/~kkharbas/8204908/webrev-
> 8204908.01_to_00/
> >
> > I have fixed all the indentation and format related comments, others I
> > have pasted here below with my comments inline.
> >
> > =============================
> > > src/hotspot/share/gc/parallel/adjoiningGenerations.hpp
> > > - Copyright update
> > >
> > > 62   AdjoiningGenerations();
> > > - Why we need this ctor?
> > >
> > >> I need this default ctor for constructing
> > adjoiningGenerationsForHeteroHeap, since I don't want to call the non-
> > default constructor of adjoiningGenerations.
> >
> > > =========================
> > > /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.
> > >
> > >> I feel overriding these methods is a good idea from design
> > >> standpoint;
> > code changes in future would take benefit of this.
> >
> > ========================
> > > src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.hpp
> > >    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().
> > >
> > >> AdjoiningVirtualSpaces contains two adjacent virtual spaces in the
> > >> same
> > reserved space and separated by a boundary. That’s why the name 'high'
> > and 'low'.
> > >> The class I added - HeteroVirtualSpaces, are not adjacent and do
> > >> not
> > share same reserved space. So I have names them '_young_vs' and
> '_old_vs'.
> > But from outside, i.e, users of VirtualSpaces, they call high() and
> > low() to access these spaces. So I have not changed the function names.
> >
> > > -----Original Message-----
> > > From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> > > Sent: Monday, September 17, 2018 11:32 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 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_si
> > > ze
> > > ,
> > > 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-runtime-dev mailing list