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

[sangheon.kim]

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.

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

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

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

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 
> <https://bugs.openjdk.java.net/browse/JDK-8171181>
>