FW: RFR(M): 8171181: Supporting heap allocation on alternative memory devices
Thomas Schatzl
thomas.schatzl at oracle.com
Mon Oct 30 08:00:41 UTC 2017
Hi,
On Thu, 2017-10-26 at 04:33 +0000, Kharbas, Kishor wrote:
> I got delivery failure to the individual email addresses, so
> forwarding the email.
>
> Regards
> Kishor
>
> -----Original Message-----
> From: Kharbas, Kishor
> Sent: Wednesday, October 25, 2017 9:28 PM
> To: Thomas Schatzl <thomas.schatzl at oracle.com>; sangheon.kim <sangheo
> n.kim at oracle.com>; 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-dev@
> openjdk.java.net>
> Cc: hotspot-runtime-dev at openjdk.java.net; Kharbas, Kishor <kishor.kha
> rbas at intel.com>
> Subject: RE: RFR(M): 8171181: Supporting heap allocation on
> alternative memory devices
>
> Hi Sangheon, Thomas,
>
> Thanks for the review. Here is the latest webrev based on your
> comments and my replies to both are inline (I copied Sangheon's email
> here at the top above Thomas's reply)
> http://cr.openjdk.java.net/~kkharbas/8171181/webrev.10_to_09
> http://cr.openjdk.java.net/~kkharbas/8171181/webrev.10
>
> Please let me know your question/comments.
Just minor nits for now:
- virtualspace.cpp:72: s/Healper/Helper
- TestAllocateHeapAt.java:2: This is a new file, so I guess copyright
date should not start from 2013.
Thanks for all these changes.
Thanks,
Thomas
>
> Thanks
> Kishor
>
> > Hi Kishor,
> > On 10/19/2017 08:00 AM, Kharbas, Kishor wrote:
> > Hi Sangheon,
>
>
> > Thanks for the review and comments.
> > I created two webrevs:
> > webrev.07 : Is the rebase of webrev.06 on jdk10 (http://cr.openj
> > dk.java.net/~kkharbas/8171181/webrev.07/)
> > webrev.08 : Is incremental webrev over 07. (http://cr.
> > openjdk.java.net/~kkharbas/8171181/webrev.08/)
> > 1. I couldn't apply webrev.07 patch cleanly. Could you test?
> > Just minor nit: 07 seems not only rebase of webrev.06. i.e.
> > there seems to have some changes from my comment of alignments.
>
> [Kharbas, Kishor] My bad, I had rebased the patch on jdk10/jdk10. The
> further ones are on jdk10/hs
>
> > 2. The description in the JEP uses 'AllocateHeapAt' while the patch
> > uses 'HeapDir'.
>
> [Kharbas, Kishor] I changed the code. Thanks
>
> > -----------------------------
> > src/os/posix/vm/os_posix.cpp
> > 159 (void)strcpy(fullname, dir);
> > - Please use strncpy instead.
>
> [Kharbas, Kishor] Done
>
> > 160 (void)strcat(fullname, name_template);
> > - Please use strncat instead.
>
> [Kharbas, Kishor] Done.
>
> > 180 ::free(fullname);
> > - os::free().
>
> [Kharbas, Kishor] Done.
>
> > 196 ::free(fullname);
> > - os::free().
>
> [Kharbas, Kishor] Done.
>
> > -----------------------------
> > src/os/windows/vm/os_windows.cpp
> > 2885 char *fullname = (char*)_alloca(strlen(dir) +
> > sizeof(name_template));
> > - Missing allocation failure handling.
> > - Please use _malloca instead.
> > * This function is deprecated because a more secure version is
> > available; see _malloca.
> > https://msdn.microsoft.com/en-us/library/wb1s57t5.aspx
>
> [Kharbas, Kishor] Done.
>
> > 2886 (void)strcpy(fullname, dir);
> > - Please use strncpy instead.
>
> [Kharbas, Kishor] Done.
>
> > 2887 (void)strcat(fullname, name_template);
> > - Please use strncat instead.
>
> [Kharbas, Kishor] Done.
>
> > -----------------------------
> > src/share/vm/runtime/arguments.cpp
> > 3726 }
> > 3727 }
> > - My previous comment was to add explicit explanation of disabling
> > UseNUMA and UseNUMAInterleaving.
> > 4636 if(!FLAG_IS_DEFAULT(HeapDir)) {
> > - Previsouly we checked UseNUMA and UseNUMAInterleaving and then
> > disabled those. IMHO, I think previous one seems better with more
> > explanation that we are going to disable those options. i.e.
> > >Similar to os_linux.cpp:4869, a warning message with disabling
> > codes.
>
> [Kharbas, Kishor] Based on our discussion offline, I think we agree
> that it’s OK to disable UseNUMA and leave UseNUMAInterleaving ON
> since other areas of JVM such as CodeCache can use
> UseNUMAInterleaving.
>
> > 4638 }
> > 4639 else if (UseParallelGC || UseParallelOldGC) {
> > - One line please if this change is still needed.
>
> -> } else if {
>
> > -----------------------------
> > src/share/vm/runtime/os.cpp
> > 1678
> > - Extra line.
>
> [Kharbas, Kishor] Done.
>
> > 1688 }
> > 1689 else {
> > -> } else {
>
> [Kharbas, Kishor] Done.
>
> > My answers are inlined.
>
>
> > In webrev08 I have addressed all the comments, except for ones
> > below:
>
>
> > ---------------------------------------
> > src/os/aix/vm/os_aix.cpp
> > 2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char*
> > requested_addr, bool use_SHM) {
> > - Question. Why os_aix has additional parameter at
> > pd_attemp_reserve_memory_at()? Probably only AIX has shmated memory
> > implementation?
> > A: Yes that’s correct.
> > Okay.
> > --------------------------------------
> > 137 log_debug(gc, heap, coops)("UseLargePages can't be set
> > with HeapDir option.");
> > - Is it enough to print log message instead of warning message?
> > i.e. Don't we need something at arguments.cpp:3656, similar to NUMA
> > flags?
> > A: If don’t disable UseLargePages like UseNUMA because large
> > pages can be used for other allocation such as CodeCache.
> > I meant to changing log_debug() to log_warning().
> > If UseNUMA is enabled, we print warning at arguments.cpp:3725 while
> > UseLargePages is enabled, we print debug message.
> > In addition, is this enough? Don't we need to force disabling
> > UseLargePages?
> > What happens if both options are enabled?
> > ------------------------------------
> > 603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t
> > alignment,
> > bool large, const char* backing_fs_for_heap)
> > - ReservedSpace has '_backing_fd' but the constructor doesn't take
> > it as a parameter and only ReservedHeapSpace uses it. This seems
> > not ideal, couldn't make it better? I know actual logic is at
> > >ReservedSpace so it is not convenient to add _backing_fs_for_heap
> > at ReservedHeapSapce.
> > A: ReservedHeapSpace depends on few functions in ReservedSpace
> > such as initialize(), release(). So instead of passing it as
> > argument, I set it as a propert of ReservedSpace.
> > Okay.
>
> -----------------------------------
> > 1663
> > - You removed os::attempt_reserve_memory_at() from os.cpp and split
> > into os_posix.cpp and os_windows.cpp.
> > But I think you should remain os::attempt_reserve_memory_at() at
> > os.cpp and implement os specific stuffs at each os_xxx.cpp files
> > for pd_xxx. Of cource move MemTracker function call as well.
> > A: I do it this way to reduce the redundant code, If I
> > implement in OS specific files in pd_xxx(), the code to replace
> > reserved mapping with file mapping
> > >(replace_existing_mapping_with_dax_file_mapping()) will be
> > redundant.
> > Still if you feel I will do the change and see how it
> > looks.
> > I would prefer to have pd_xxx() even though there's some redundant
> > code.
>
> [Kharbas, Kishor] We also had a discussion about this offline, I will
> address this in the next webrev.
>
> > Thanks,
> > Sangheon
> > -----Original Message-----
> > From: Thomas Schatzl [mailto:thomas.schatzl at oracle.com]
> > Sent: Tuesday, October 24, 2017 8:39 AM
> > To: Kharbas, Kishor <kishor.kharbas at intel.com>; sangheon.kim
> > <sangheon.kim at oracle.com>; 'hotspot-gc-dev at openjdk.java.net'
> > <hotspot-gc-dev at openjdk.java.net>
> > Cc: hotspot-runtime-dev at openjdk.java.net
> > Subject: Re: RFR(M): 8171181: Supporting heap allocation on
> > alternative memory devices
> >
> > Hi Kishor,
> >
> > initial review using the webrevs, and the comment thread in
> > hotspot-
> > runtime-dev (readded to cc because questions were left unanswered
> > there).
> >
> > The code also touches runtime code quite a bit, so I would
> > appreciate
> > comments by the runtime team.
> >
> > On Thu, 2017-10-19 at 15:00 +0000, Kharbas, Kishor wrote:
> > > Hi Sangheon,
> > >
> > > Thanks for the review and comments.
> > > I created two webrevs:
> > > webrev.07 : Is the rebase of webrev.06 on jdk10
> > > (http://cr.openjdk
> > > .java.net/~kkharbas/8171181/webrev.07/)
> > > webrev.08 : Is incremental webrev over 07.
> > > (http://cr.op
> > > enjdk.java.net/~kkharbas/8171181/webrev.08/)
> > >
> >
> > Some first comments on the webrev follow:
> >
> > - could you please provide both a full and incremental webrev for
> > your changes? The former help the reviewers late to the party, the
> > incremental ones the people already working with you.
> >
>
> [Kharbas, Kishor] Yes. I will keep this in mind.
>
> > - the patch has not been rebased to jdk10/hs as Sangheon
> > suggested.
> > Probably you rebased to jdk10/jdk10?
> >
>
> [Kharbas, Kishor] My bad, it was rebased to jdk10/jdk10. New webrev's
> will be based on jdk10/hs.
>
> > os_posix.cpp:
> >
> > - os::create_file_for_heap: the malloc allocates one byte too
> > little,
> > creating an automatic write outside the buffer at the strcat()
> > method.
> >
>
> [Kharbas, Kishor] Done.
>
> > - os::create_file_for_heap does not use the size parameter. Please
> > remove.
> >
>
> [Kharbas, Kishor] Done.
>
> > - in several places: improve the output of the error messages to
> > have
> > meaning for the user. Not just "malloc failed"; there were already
> > some suggestions in the referenced thread at
> > http://mail.openjdk.java.net/p ipermail/hotspot-runtime-dev/2017-
> > March/022733.html .
> >
> > Same for the asserts, and particularly for them - they will
> > implicitly
> > terminate the VM, so e.g. a "fchmod error" is definitely too
> > little.
> > At least error no/output of os::strerror() etc. would be required
> > for quick diagnosis.
> >
> > I read in some of the emails that you intend to let the caller
> > print a
> > good error message. I fear that the caller might not have the
> > necessary information any more to do that in a useful way.
> >
>
> [Kharbas, Kishor] Done. Moved macro assert_with_errror() to top of
> file so I can use in my code.
>
> > - in that same thread there has also been the question about the
> > need
> > for blocking the signals for the process that has gone unanswered.
> >
>
> [Kharbas, Kishor] Let me get back on this. The implementation at
> pmem.io which I use as reference for file operations does blocks
> signals. I will check if there is more to it than just a 'good
> practice'.
>
> > - in that same thread there has also been the question about why
> > the
> > code sets permissions to 0600 manually; this is because of glibc
> > 2.0.6 compatibility.
> > Glibc 2.06 has been released 1997-12-29. I looked a bit through
> > glibc
> > requirements for the Oracle JDK, and while I could not find exact
> > requirements, some posts indicate the need for GLIBC_2.4 at
> > minimum
> > (for
> > jdk7) which is from 2003. I recommend removing this, it seems
> > unnecessary and completely outdated.
> >
>
> [Kharbas, Kishor] Agree. Removed it.
>
> > - os::malloc should be paired with os::free in
> > os::create_file_for_heap() (2x)
> >
>
> [Kharbas, Kishor] Done.
>
> > - I am not sure whether the methods that deal with mapping memory
> > to
> > a file should have "dax" in their name. The code seems to be
> > pretty
> > generically map memory into a file.
> >
>
> [Kharbas, Kishor] I used 'dax' in the name because to get direct
> mapping to DAX device you need to use 'MAP_SHARED'. That makes this
> map function different than generic map function. But it’s a minor
> point. I can change the name if it's still desired./mkstem/
>
> > - I am not sure if the current approach to FS errors after mkstemp
> > is
> > very nice. Please at least print a meaningful warning message to
> > the log.
> >
>
> [Kharbas, Kishor] Added a warning() message when mkstemp could not
> create a file.
>
> > - please also specify the meaning of the return value for
> > os::create_file_for_heap() in the documentation.
> >
>
> [Kharbas, Kishor] Added more information in the comment before
> function's declaration.
>
> > - there is a missing "p" in the name of
> > os::reserve_mmaped_memory().
> >
>
> [Kharbas, Kishor] Done.
>
> > - os::util_posix_fallocate(): s/continous/continuous
> >
>
> [Kharbas, Kishor] Done.
>
> > - os::map_memory_to_dax_file(): according to man pages,
> > posix_fallocate does not set errno, but the code checking its
> > return
> > (or
> > util_posix_fallocate()) expects it to do so.
> >
>
> [Kharbas, Kishor] I removed printing of error string. Can the return
> error value of posix_fallocate() we used as argument to
> os::strerror(errno)?
>
> > - os::attempt_reserve_memory_at(), in the call to
> > pd_attempt_reserve_memory_at() for AIX, in the third parameter,
> > please
> > use the exact name of the parameter in the comment.
> >
>
> [Kharbas, Kishor] Done.
>
> > - os::attempt_reserve_memory_at(), the second "if" needs a blank
> > before the bracket.
> >
>
> [Kharbas, Kishor] Done.
>
> > - os::attempt_reserve_memory_at(), in the comment,
> > s/mmemory/memory
> >
>
> [Kharbas, Kishor] Done.
>
> > - in that same method, the #if..#endif block should be aligned
> > like
> > other similar blocks.
> >
>
> [Kharbas, Kishor] Done.
>
> > - in that same method, the "else" should be on the same line as
> > the
> > closing bracket of the if-body.
> >
>
> [Kharbas, Kishor] Done.
>
> > os_windows.cpp:
> >
> > os::create_file_for_heap():
> > - same bug about length of allocation the _alloca call. Not
> > completely sure about why use alloca (or not use alloca in
> > os_posix.cpp).
> >
>
> [Kharbas, Kishor] Changed to have uniform implementation.
>
> > - this version calls os::native_path(). It seems strange to not
> > do
> > that as well in the others (although it does nothing).
> >
>
> [Kharbas, Kishor] Added in os_posix.cpp to make it uniform.
>
> > - os::reserve_memory_aligned(): an "else" should be moved into
> > the
> > same line as the closing bracket.
> >
>
> [Kharbas, Kishor] Done.
>
> > universe.cpp:
> >
> > - Universe::reserve_heap(): I do not think this functionality has
> > anything to do with compressed oops, so the log message should not
> > have the coops tag.
> > The log message could/should have more information about the file
> > location.
> > I also recommend using "Java heap" instead of "Heap" here, as the
> > latter is ambiguous.
> >
>
> [Kharbas, Kishor] Done. Added more info to log message.
>
> > virtualspace.cpp:
> >
> > - in failed_to_reserve_as_requested: "else" indentation. Also,
> > the
> > "else { if (..." could be shortened to "} else if (..." to avoid
> > the extra indentation level.
> > (2x)
> >
>
> [Kharbas, Kishor] Done.
>
> > - ReservedSpace::initialize(): s/upto/up to
> >
>
> [Kharbas, Kishor] Done.
>
> > - ReservedSpace::initialize(): remove the coops tag here as well.
> > Also, the info message might be more similar to the comment above
> > where it says that large page support is up to the file system, and
> > the flag ignored.
> >
>
> [Kharbas, Kishor] Done.
> > - the code contains the snippet
> >
> > if (_fd_for_heap != -1) {
> > if (!os::unmap_memory...
> > } else {
> > if (!os::release_memory...
> > }
> >
> > at least twice. Maybe this code snippet could be extracted into a
> > new
> > method.
> >
>
> [Kharbas, Kishor] Defined a new static function
> unmap_or_release_memory()
>
> > - (ignore if you want) ReservedHeapSpace::ReservedHeapSpace, at
> > the
> > end
> > - maybe the condition is easier to read if it looks like:
> >
> > if (_fd_for_heap != -1) {
> > os::close(_fd_for_heap);
> > }
> >
> > than it is now.
>
> [Kharbas, Kishor] I agree. Done.
> >
> > - virtualspace.hpp: in the changed constructor signature, please
> > add
> > a comment about what backing_fs_for_heap actually means (and what
> > happens if set).
> >
>
> [Kharbas, Kishor] Done.
>
> > arguments.cpp:
> >
> > - Arguments::finalize_vm_init_args: maybe at the place where the
> > change adds the warning/info message about NUMA support being
> > specific
> > to the file system; also add the same warning about UseLargePages
> > that
> > is located elsewhere.
> >
> > Like "UseXXXX may not be supported in some specific file system
> > implementations." - or just ignore as Andrew said in the other
> > email thread.
> >
> > Note that I am not sure that the selected place is the correct
> > place
> > for such warning about incompatible flags (and maybe
> > UseNUMA/UseLargePages should be forcibly disabled here? But then
> > again, it does not hurt just having it enabled?).
> >
> > I will let the runtime team comment as a lot of that argument
> > processing changed in jdk9.
> >
> > Maybe Arguments::check_vm_args_consistency() is better.
> >
> > There is similar code in Arguments::adjust_after_os()...
> >
>
> [Kharbas, Kishor] In progress. I will get back on these comments in
> next reply.
>
> > os.cpp:
> >
> > - os::reserve_memory: "else" indentation
> >
> > - os::reserve_memory: I do not follow that comment - it explains
> > the
> > code as written, not why, and what map_memory_to_dax_file() has to
> > do
> > with AIX and SHM. It uses an outdated method name, and also
> > s/your/our.
> >
> > Probably the whole comment can be removed.
> >
>
> [Kharbas, Kishor] I changed the comment. In the beginning I had
> called pd_reserve_memory() followed by
> replace_existing_mapping_with_dax_file_mapping(), but later realized
> that AIX can allocate from SHM, in which case it's not straight
> forward to replace the mapping. So I leave this comment to make sure
> in future I (or someone else) does not ponder over the same thing.
>
> > os.hpp:
> >
> > - indentation of the new #if defined clause
> >
> > - here I may probably be speaking wrongly on behalf of the
> > runtime
> > team, but os.hpp, as an abstraction of the OS should probably not
> > have
> > platform specific ifdefs in it.
> >
>
> [Kharbas, Kishor] Change the indentation. I will address abstraction
> issue in next reply.
>
> > Thanks,
> > Thomas
> >
> > > In webrev08 I have addressed all the comments, except for ones
> > > below:
> > >
> > > ---------------------------------------
> > > src/os/aix/vm/os_aix.cpp
> > >
> > > 2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char*
> > > requested_addr, bool use_SHM) {
> > > - Question. Why os_aix has additional parameter at
> > > pd_attemp_reserve_memory_at()? Probably only AIX has shmated
> >
> > memory
> > > implementation?
> > > A: Yes that’s correct.
> > >
> > > --------------------------------------
> > > 137 log_debug(gc, heap, coops)("UseLargePages can't be set
> > > with HeapDir option.");
> > > - Is it enough to print log message instead of warning message?
> > > i.e.
> > > Don't we need something at arguments.cpp:3656, similar to NUMA
> > > flags?
> > > A: If don’t disable UseLargePages like UseNUMA because
> > > large
> > > pages can be used for other allocation such as CodeCache.
> > >
> > > ------------------------------------
> > >
> > > 603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t
> > > alignment, bool large, const char* backing_fs_for_heap)
> > > - ReservedSpace has '_backing_fd' but the constructor doesn't
> > > take
> > > it as a parameter and only ReservedHeapSpace uses it. This seems
> > > not
> > > ideal, couldn't make it better? I know actual logic is at
> > > ReservedSpace so it is not convenient to add _backing_fs_for_heap
> > > at
> > > ReservedHeapSapce.
> > > A: ReservedHeapSpace depends on few functions in
> > > ReservedSpace
> > > such as initialize(), release(). So instead of passing it as
> > > argument, I set it as a propert of ReservedSpace.
> > >
> > > -----------------------------------
> > > 1663
> > > - You removed os::attempt_reserve_memory_at() from os.cpp and
> > > split
> > > into os_posix.cpp and os_windows.cpp.
> > > But I think you should remain os::attempt_reserve_memory_at()
> > > at
> > > os.cpp and implement os specific stuffs at each os_xxx.cpp files
> > > for
> > > pd_xxx. Of cource move MemTracker function call as well.
> > > A: I do it this way to reduce the redundant code, If I
> > > implement in OS specific files in pd_xxx(), the code to replace
> > > reserved mapping with file mapping
> > > (replace_existing_mapping_with_dax_file_mapping()) will be
> > > redundant.
> > > Still if you feel I will do the change and see how
> > > it
> > > looks.
> > >
> > > Regards
> > > Kishor
> > >
> > >
> > > From: sangheon.kim [mailto:sangheon.kim at oracle.com]
> > > Sent: Tuesday, September 26, 2017 3:18 PM
> > > To: Kharbas, Kishor <kishor.kharbas at intel.com>;
> > > 'hotspot-gc-dev at openj dk.java.net' <hotspot-gc-dev at openjdk.java.n
> > > et>
> > > Subject: Re: RFR(M): 8171181: Supporting heap allocation on
> > > alternative memory devices
> > >
> > > Hi Kishor,
> > >
> > > On 07/20/2017 06:34 PM, Kharbas, Kishor wrote:
> > > I have a new version of this patch at
> > > http://cr.openjdk.java.net/~kkh arbas/8171181/webrev.06/
> > >
> > > This version has been tested on Windows, Linux, Solaris and Mac
> > > OS.
> > > I could not get access to AIX for testing.
> > > I used tmpfs to test the functionality. Cases that were tested
> > > were.
> > > 1. Allocation of heap using file mapping when –XX:HeapDir=
> > > option is used.
> > > 2. Creation of nameless temporary file for Heap allocation
> > > which prevents access to file using its name.
> > > 3. Correct deletion and freeing up of space allocated for
> > > file
> > > under different exit conditions.
> > > 4. Error handling when path specified is not present, heap
> > > size is more than size of file system, etc.
> > > Overall seems good.
> > > I tried to list some missing part.
> > >
> > > 1. Please rebase with consolidated repository. (jdk10/hs) 2.
> > > Build
> > > failure on Solaris.
> > > (Sorry no build error log file, as I tested a few weeks ago,
> > > it
> > > is
> > > deleted) 3. Have you tested with various heap reserve path using
> > > HeapBaseMinAddress flag? i.e. to test code path of
> > > ReservedHeapSpace::try_reserve_heap() and try_reserve_range().
> > > 4. How about adding HeapDir allocation success message? e.g.
> > > gc+heap+coops=info
> > > 5. Adding JTREG test. Probably we would need to verify this
> > > allocation is succeeded via #4 added allocation success message.
> > > 6. CSR (Compatibility & Specification Review). At some point,
> > > you
> > > need to file another CR of 'CSR' type to add a new flag of
> > > 'HeapDir'.
> > > 7. It will be much appreciated if you provide incremental webrev.
> > > I
> > > think 06(this version) vs. 07(rebase version) would be hard to
> > > get.
> > > Probably from 08 version.
> > >
> > > Here's my comments.
> > > -----------------------------
> > > src/os/aix/vm/os_aix.cpp
> > >
> > > 2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char*
> > > requested_addr, bool use_SHM) {
> > > - Question. Why os_aix has additional parameter at
> > > pd_attemp_reserve_memory_at()? Probably only AIX has shmated
> >
> > memory
> > > implementation?
> > >
> > > -----------------------------
> > > src/os/posix/vm/os_posix.cpp
> > >
> > > 148 char *fullname = (char*)::malloc(strlen(dir) +
> > > sizeof(name_template));
> > > - Use os::malloc()
> > >
> > > 196 int flags;
> > > 197
> > > 198 flags = MAP_PRIVATE | MAP_NORESERVE | MAP_ANONYMOUS;
> > > - Combining 196 and 198 seems better to me.
> > >
> > > 200 assert((uintptr_t)requested_addr % os::Linux::page_size()
> > > ==
> > > 0, "unaligned address");
> > > - Linux dependency on posix file which makes build error on
> > > Solaris.
> > > Probably os::vm_page_size().
> > >
> > > 207 addr = (char*)::mmap(requested_addr, bytes, PROT_NONE,
> > > 208 flags, -1, 0);
> > > - Missing some spaces? Alignment seems odd to me.
> > >
> > > 226 if (ret == -1)
> > > - Probably you wanted to add handling code? If not, just return
> > > ret.
> > >
> > > 252 if (addr == MAP_FAILED || (base != NULL && addr != base)) {
> > > 253 if (addr != MAP_FAILED) {
> > > 254 if (!os::release_memory(addr, size)) {
> > > 255 warning("Could not release memory on unsuccessful
> > > file
> > > mapping");
> > > 256 }
> > > 257 }
> > > 258 return NULL;
> > > 259 }
> > > - Splitting MAP_FAILED case and another gives better readability
> > > to
> > > me. But this is your call.
> > >
> > > 269
> > > - Extra line.
> > >
> > > 284 if (result != NULL && file_desc != -1) {
> > > 285 if
> > > (replace_existing_mapping_with_dax_file_mapping(result,
> > > bytes, file_desc) == NULL) {
> > > 286 vm_exit_during_initialization(err_msg("Error in
> > > mapping
> > > Java heap at the given filesystem directory"));
> > > 287 }
> > > 288
> > >
> >
> > MemTracker::record_virtual_memory_reserve_and_commit((address)resul
> > t
> > ,
> > > bytes, CALLER_PC);
> > > 289 return result;
> > > 290 }
> > > 291 if (result != NULL) {
> > > 292
> > > MemTracker::record_virtual_memory_reserve((address)result,
> > > bytes, CALLER_PC);
> > > 293 }
> > > - Combining line 284 and 291 seems better to me.
> > > 284 if (result != NULL) {
> > > if (file_desc != -1) {
> > > if
> > > (replace_existing_mapping_with_dax_file_mapping(result,
> > > bytes, file_desc) == NULL) {
> > > vm_exit_during_initialization(err_msg("Error in
> > > mapping
> > > Java heap at the given filesystem directory"));
> > > }
> > >
> > >
> >
> > MemTracker::record_virtual_memory_reserve_and_commit((address)resul
> > t
> > ,
> > > bytes, CALLER_PC);
> > > } else {
> > >
> > > MemTracker::record_virtual_memory_reserve((address)result,
> > > bytes, CALLER_PC);
> > > }
> > > }
> > > return result;
> > >
> > > -----------------------------
> > > src/os/windows/vm/os_windows.cpp
> > > 3141 // if 'base' is not NULL, function will return NULL if it
> > > cannot get 'base'
> > > - Start with uppercase.
> > >
> > > 3142 //
> > > - This line seems redundant.
> > >
> > > 3151 vm_exit_during_initialization(err_msg("Could not
> > > allocate
> > > sufficient disk space for heap"));
> > > - heap -> Java heap (same as line 3153)
> > >
> > > 3168 assert(base != NULL, "base cannot be NULL");
> > > - 'base' -> 'Base' or 'Base address'.
> > >
> > > 3172
> > > - Redundant line.
> > >
> > > 3230 }
> > > 3231 else {
> > > -> } else {
> > >
> > > 3278 return reserve_memory(bytes, requested_addr, 0);
> > > - Is it correct to use '0' not '-1'? It would be better to
> > > explain
> > > why we use hard-coded value here.
> > >
> > > -----------------------------
> > > src/share/vm/memory/universe.cpp
> > > - No comments
> > >
> > > -----------------------------
> > > src/share/vm/memory/virtualspace.cpp
> > > - copyright update
> > >
> > > 74 const size_t size,
> > > bool special, bool is_file_mapped= false)
> > > - Need space between 'is_file_mapped' and '='.
> > >
> > > 92 fatal("os::release_memory failed");
> > > - Typo, 'os::unmap_memory failed'.
> > >
> > > 129 // If there is a backing file directory for this
> > > VirtualSpace
> > > then whether
> > > - This is not VirtualSpace. Probably just 'space'.
> > >
> > > 130 // large pages are allocated is upto the filesystem the
> > > dir
> > > resides in.
> > > - 'dir'? Probably 'backing file for Java heap'.
> > >
> > > 137 log_debug(gc, heap, coops)("UseLargePages can't be set
> > > with HeapDir option.");
> > > - Is it enough to print log message instead of warning message?
> > > i.e.
> > > Don't we need something at arguments.cpp:3656, similar to NUMA
> > > flags?
> > >
> > > 191 // unmap_memory will do extra work esp. in Windows
> > > - esp. -> especially
> > >
> > > 282 }
> > > 283 else {
> > > -> } else {
> > >
> > > 346 // If there is a backing file directory for this
> > > VirtualSpace
> > > then whether
> > > - Again this is not VirtualSpace, so just 'space'.
> > >
> > > 352 if (UseLargePages && (!FLAG_IS_DEFAULT(UseLargePages) ||
> > > 353 !FLAG_IS_DEFAULT(LargePageSizeInBytes))) {
> > > - Wrong alignment at line 353. Consider to make same as line 380.
> > >
> > > 603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t
> > > alignment, bool large, const char* backing_fs_for_heap)
> > > - ReservedSpace has '_backing_fd' but the constructor doesn't
> > > take
> > > it as a parameter and only ReservedHeapSpace uses it. This seems
> > > not
> > > ideal, couldn't make it better? I know actual logic is at
> > > ReservedSpace so it is not convenient to add _backing_fs_for_heap
> > > at
> > > ReservedHeapSapce.
> > >
> > > -----------------------------
> > > src/share/vm/memory/virtualspace.hpp
> > > 40 int _backing_fd;
> > > - I would prefer to have better name to describe.
> > > e.g. as command-line option name is 'HeapDir', _heap_fd or
> > > _fd_for_heap(similar to below)?
> > >
> > > 115 ReservedHeapSpace(size_t size, size_t
> > > forced_base_alignment,
> > > bool large, const char* backingFSforHeap = NULL);
> > > - Snake case. How about 'fs_for_heap' or 'heap_fs'?
> > >
> > > -----------------------------
> > > src/share/vm/runtime/arguments.cpp
> > > 3655 FLAG_SET_CMDLINE(bool, UseNUMA, false);
> > > - (questions) Don't need to add a warning message for
> > > UseLargePagesSame=true as commented virtualspace.cpp:137?
> > >
> > > -----------------------------
> > > src/share/vm/runtime/globals.hpp
> > > - No comments
> > >
> > > -----------------------------
> > > src/share/vm/runtime/os.cpp
> > >
> > > 1632
> > > - Extra line.
> > >
> > > 1642 }
> > > 1643 else {
> > > -> } else {
> > >
> > > 1663
> > > - You removed os::attempt_reserve_memory_at() from os.cpp and
> > > split
> > > into os_posix.cpp and os_windows.cpp.
> > > But I think you should remain os::attempt_reserve_memory_at()
> > > at
> > > os.cpp and implement os specific stuffs at each os_xxx.cpp files
> > > for
> > > pd_xxx. Of cource move MemTracker function call as well.
> > >
> > > -----------------------------
> > > src/share/vm/runtime/os.hpp
> > >
> > > 349 // replace existing reserved memory with file mapping
> > > - Start with uppercase letter.
> > >
> > > Thanks,
> > > Sangheon
> > >
> > >
> > >
> > >
> > > - Kishor
> > >
> > > From: Kharbas, Kishor
> > > Sent: Tuesday, July 11, 2017 6:40 PM
> > > To: 'hotspot-gc-dev at openjdk.java.net'
> > > <hotspot-gc-dev at openjdk.java.ne
> > > t>
> > > Cc: Kharbas, Kishor <kishor.kharbas at intel.com>
> > > Subject: RFR(M): 8171181: Supporting heap allocation on
> > > alternative
> > > memory devices
> > >
> > > Greetings,
> > >
> > > I have an updated patch for JEP
> > > https://bugs.openjdk.java.net/browse/
> > > JDK-8171181 at
> > > http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05
> > > This patch fixes the bugs pointed earlier and other suggestions
> > > to
> > > make the code less intrusive.
> > >
> > > I have also sent this to ‘hotspot-runtime-dev’ mailing list
> > > (included below).
> > >
> > > I would appreciate comments and feedback.
> > >
> > > Thanks
> > > Kishor
> > >
> > > From: Kharbas, Kishor
> > > Sent: Monday, July 10, 2017 1:53 PM
> > > To: hotspot-runtime-dev at openjdk.java.net
> > > Cc: Kharbas, Kishor <kishor.kharbas at intel.com>
> > > Subject: RFR(M): 8171181: Supporting heap allocation on
> > > alternative
> > > memory devices
> > >
> > > Hello all!
> > >
> > > I have an updated patch for
> > > https://bugs.openjdk.java.net/browse/JDK-
> > > 8171181 at http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05
> > > I have lost the old email chain so had to start a fresh one. The
> > > archived conversation can be found at -
> > > http://mail.openjdk.java.net/
> > > pipermail/hotspot-runtime-dev/2017-March/022733.html
> > >
> > > 1. I have worked on all the comments and fixed the bugs.
> > > Mainly bugs fixed are related to sigprocmask() and changed the
> > > implementation such that ‘fd’ is not passed all the way down the
> > > call stack. Thus minimizing function signature changes.
> > >
> > > 2. Patch supports all OS’es. Consolidated all Posix
> > > compliant
> > > OS’s implementation in os_posix.cpp.
> > >
> > > 3. The patch is tested on Windows and Linux. Working on
> > > testing it on other OS’es.
> > >
> > > Let me know if this version looks clean and correct.
> > >
> > > Thanks
> > > Kishor
> > >
>
>
More information about the hotspot-runtime-dev
mailing list