RFR(M): 8190308: Supporting heap allocation on alternative memory devices and CSR review

[Kharbas, Kishor]

Hi Sangheon,

Thanks for the review and comments. Here is an updated webrev-
http://cr.openjdk.java.net/~kkharbas/8190308/webrev.12
http://cr.openjdk.java.net/~kkharbas/8190308/webrev.12_to_11

In addition to your suggested corrections, I added code to set Linux core dump filter ensuring Heap is dumped correctly when this feature is used. This is follow-up to Jini George's comment (http://openjdk.5641.n7.nabble.com/RFR-M-8171181-Supporting-heap-allocation-on-alternative-memory-devices-td300109.html#a300450).

(some reply is inline)

Thanks
Kishor
From: sangheon.kim [mailto:sangheon.kim at oracle.com]
Sent: Wednesday, November 1, 2017 10:53 PM
To: Kharbas, Kishor <kishor.kharbas at intel.com>; 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>; hotspot-runtime-dev at openjdk.java.net
Cc: Thomas Schatzl <thomas.schatzl at oracle.com>
Subject: Re: RFR(M): 8190308: Supporting heap allocation on alternative memory devices and CSR review

Hi Kishor,
On 10/31/2017 04:53 PM, Kharbas, Kishor wrote:

Greetings,

I am continuing the earlier discussion [1] with a revised issue number representing the implementation of the JEP [2].

I have addressed the remaining unaddressed comments (copied below) in these webrevs -
http://cr.openjdk.java.net/~kkharbas/8190308/webrev.11/<http://cr.openjdk.java.net/%7Ekkharbas/8190308/webrev.11/>
http://cr.openjdk.java.net/~kkharbas/8190308/webrev.11_to_10/<http://cr.openjdk.java.net/%7Ekkharbas/8190308/webrev.11_to_10/>

Also, I would appreciate a review of the CSR for the new flag at https://bugs.openjdk.java.net/browse/JDK-8190309.
CSR: Reviewed.





> >  - in that same thread there has also been the question about the

> > need

> > for blocking the signals for the process that has gone unanswered.

> >



Removed the blocking of signals.



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

> >



1. Moved the check from finalize_vm_init_args() to check_vm_args_consistency()

2. When UseNUMA flag is true, adaptive logical group chunk resizing is used for Numa aware collectors such as ParallelOld. Just like UseNUMA is disabled in os::inti_2() in Linux when UseLargePages is set, it will be a good idea to disable UseNUMA when the new option is used.

3. I think its ok to leave UseNUMAInterleaving ON as it can be used for other memory areas.

4. For the same reason I also do not disable UseLargePages.

5. About handling UseLargePages, I thought of handling it the same way as when reserve_memory_special() fails to allocate large pages, i.e. generating a log_debug message.

      // failed; try to reserve regular memory below

  if (UseLargePages && (!FLAG_IS_DEFAULT(UseLargePages) ||

                        !FLAG_IS_DEFAULT(LargePageSizeInBytes))) {

    log_debug(gc, heap, coops)("Reserve regular memory without large pages");




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

> >
and

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


In the updated webrev, I move the implementation from os_posix.cpp and os_window.cpp to respective pd_xxx functions. No AIX specific ifdef is required now.
Thank you for all changes.

I have minor nits now:
==============================================
- os***.cpp has some function names which include *dax*. I would prefer not to include it. As you know, Thomas also pointed it.

[Kharbas, Kishor] Done.

==============================================
src/hotspot/share/runtime/arguments.cpp
2537     if (!FLAG_IS_DEFAULT(UseNUMAInterleaving) || !FLAG_IS_DEFAULT(UseNUMA)) {
- Don't we need to check these 2 flags' value to be true? i.e. if user sets to false, below message will be printed.

[Kharbas, Kishor] That's correct. I changed it such that you check the flag is true and it's not default. In future if these flags become 'true' by default, we may not want to print warning.

==============================================
test/hotspot/jtreg/gc/TestAllocateHeapAt.java
- On other discussion, I mentioned to test only for Windows and Linux as the JEP described only about those 2. But without *dax* function names, it seems like not filtering OS seems okay too.

2  * Copyright (c) 2017, xxx Oracle and/or its affiliates. All rights reserved.
- Please remove 'xxx '.

47     Collections.addAll(vmOpts, new String[] {"-XX:AllocateHeapAt="+test_dir,
- Add spaces. '+test_dir' -> ' + test_dir'

49                                              "-Xmx50m",
50                                              "-Xms50m",
- You said there were no special reason for 200m(heap size of webrev.10) on other discussion. I would recommend 32m.

[Kharbas, Kishor] All done.

FYI, I ran hs-tier1 and hs-tier2 with your patch and the result is good. i.e. no new failures.

[Kharbas, Kishor] That's great. Thanks!

Thanks,
Sangheon








Thank you and looking forward for your feedback.

- Kishor


[1] http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-October/020682.html
[2] https://bugs.openjdk.java.net/browse/JDK-8171181


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20171103/d474e7b5/attachment.htm>