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

sangheon.kim sangheon.kim at oracle.com
Tue Sep 26 22:18:11 UTC 2017


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/ 
> <http://cr.openjdk.java.net/%7Ekkharbas/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:* 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>
> *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 
> <https://bugs.openjdk.java.net/browse/JDK-8171181> at 
> http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05 
> <http://cr.openjdk.java.net/%7Ekkharbas/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 
> <mailto:hotspot-runtime-dev at openjdk.java.net>
> *Cc:* Kharbas, Kishor <kishor.kharbas at intel.com 
> <mailto:kishor.kharbas at intel.com>>
> *Subject:* RFR(M): 8171181: Supporting heap allocation on alternative 
> memory devices
>
> Hello all!
>
> I have an updated patch forhttps://bugs.openjdk.java.net/browse/JDK-8171181 at 
> http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05 
> <http://cr.openjdk.java.net/%7Ekkharbas/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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20170926/c992ba27/attachment.htm>


More information about the hotspot-gc-dev mailing list