RFR(M): 8171181: Supporting heap allocation on alternative memory devices

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(M): 8171181: Supporting heap allocation on alternative memory devices

Kharbas, Kishor

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: [hidden email]
Cc: Kharbas, Kishor <[hidden email]>
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

Reply | Threaded
Open this post in threaded view
|

RE: RFR(M): 8171181: Supporting heap allocation on alternative memory devices

Kharbas, Kishor

I have a new version of this patch at http://cr.openjdk.java.net/~kkharbas/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.

 

- Kishor

 

From: Kharbas, Kishor
Sent: Tuesday, July 11, 2017 6:40 PM
To: '[hidden email]' <[hidden email]>
Cc: Kharbas, Kishor <[hidden email]>
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: [hidden email]
Cc: Kharbas, Kishor <[hidden email]>
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

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8171181: Supporting heap allocation on alternative memory devices

sangheon.kim@oracle.com
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/~kkharbas/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)result, 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)result, 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: '[hidden email]' [hidden email]
Cc: Kharbas, Kishor [hidden email]
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: [hidden email]
Cc: Kharbas, Kishor <[hidden email]>
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


Reply | Threaded
Open this post in threaded view
|

RE: RFR(M): 8171181: Supporting heap allocation on alternative memory devices

Kharbas, Kishor

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.openjdk.java.net/~kkharbas/8171181/webrev.08/)

 

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:[hidden email]]
Sent: Tuesday, September 26, 2017 3:18 PM
To: Kharbas, Kishor <[hidden email]>; '[hidden email]' <[hidden email]>
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/~kkharbas/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)result, 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)result, 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: '[hidden email]' [hidden email]
Cc: Kharbas, Kishor [hidden email]
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: [hidden email]
Cc: Kharbas, Kishor <[hidden email]>
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

 

Reply | Threaded
Open this post in threaded view
|

RE: RFR(M): 8171181: Supporting heap allocation on alternative memory devices

Kharbas, Kishor

Hi,

Here is an incremental webrev adding a JTReg test - http://cr.openjdk.java.net/~kkharbas/8171181/webrev.09/

Regards

Kishor

 

 

 

From: Kharbas, Kishor
Sent: Thursday, October 19, 2017 8:01 AM
To: sangheon.kim <[hidden email]>; '[hidden email]' <[hidden email]>
Cc: Kharbas, Kishor <[hidden email]>
Subject: RE: RFR(M): 8171181: Supporting heap allocation on alternative memory devices

 

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.openjdk.java.net/~kkharbas/8171181/webrev.08/)

 

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 [[hidden email]]
Sent: Tuesday, September 26, 2017 3:18 PM
To: Kharbas, Kishor <[hidden email]>; '[hidden email]' <[hidden email]>
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/~kkharbas/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)result, 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)result, 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: '[hidden email]' [hidden email]
Cc: Kharbas, Kishor [hidden email]
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: [hidden email]
Cc: Kharbas, Kishor <[hidden email]>
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

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8171181: Supporting heap allocation on alternative memory devices

sangheon.kim@oracle.com
In reply to this post by Kharbas, 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.openjdk.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.
2. The description in the JEP uses 'AllocateHeapAt' while the patch uses 'HeapDir'.

-----------------------------
src/os/posix/vm/os_posix.cpp
159   (void)strcpy(fullname, dir);
- Please use strncpy instead.

160   (void)strcat(fullname, name_template);
- Please use strncat instead.

180     ::free(fullname);
- os::free().

196   ::free(fullname);
- os::free().

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

2886   (void)strcpy(fullname, dir);
- Please use strncpy instead.

2887   (void)strcat(fullname, name_template);
- Please use strncat instead.

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

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.

1688   }
1689   else {
-> } else {

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.

Thanks,
Sangheon


 

Regards

Kishor

 

 

From: sangheon.kim [[hidden email]]
Sent: Tuesday, September 26, 2017 3:18 PM
To: Kharbas, Kishor [hidden email]; '[hidden email]' [hidden email]
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/~kkharbas/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)result, 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)result, 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: '[hidden email]' [hidden email]
Cc: Kharbas, Kishor [hidden email]
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: [hidden email]
Cc: Kharbas, Kishor <[hidden email]>
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