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.org/pipermail/hotspot-gc-dev/attachments/20170926/c992ba27/attachment.htm>
More information about the hotspot-gc-dev
mailing list