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

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

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

Kharbas, Kishor

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/~kkharbas/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.


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

Reply | Threaded
Open this post in threaded view
|

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

sangheon.kim@oracle.com
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


Reply | Threaded
Open this post in threaded view
|

RE: 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:[hidden email]]
Sent: Wednesday, November 1, 2017 10:53 PM
To: Kharbas, Kishor <[hidden email]>; '[hidden email]' <[hidden email]>; [hidden email]
Cc: Thomas Schatzl <[hidden email]>
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


Reply | Threaded
Open this post in threaded view
|

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

Thomas Schatzl
Hi,

On Fri, 2017-11-03 at 08:55 +0000, Kharbas, Kishor wrote:

> 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 minor nits:

 - os_posix.cpp:300: please move the else next to the brace
 - arguments.cpp:4624: please add a space between "if" and the bracket

I do not need to see a new webrev for these changes. Looks good.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

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

Kharbas, Kishor
Thanks a lot!

Link to updated webrevs -
http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13/
http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13_to_12/

@Sangheon: Please let me know if you see any corrections needed.

-Kishor

> -----Original Message-----
> From: Thomas Schatzl [mailto:[hidden email]]
> Sent: Friday, November 3, 2017 7:31 AM
> To: Kharbas, Kishor <[hidden email]>; sangheon.kim
> <[hidden email]>; '[hidden email]'
> <[hidden email]>; hotspot-runtime-
> [hidden email]
> Subject: Re: RFR(M): 8190308: Supporting heap allocation on alternative
> memory devices and CSR review
>
> Hi,
>
> On Fri, 2017-11-03 at 08:55 +0000, Kharbas, Kishor wrote:
> > 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 minor nits:
>
>  - os_posix.cpp:300: please move the else next to the brace
>  - arguments.cpp:4624: please add a space between "if" and the bracket
>
> I do not need to see a new webrev for these changes. Looks good.
>
> Thanks,
>   Thomas

Reply | Threaded
Open this post in threaded view
|

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

sangheon.kim@oracle.com
Hi Kishor,

On 11/03/2017 11:39 AM, Kharbas, Kishor wrote:
> Thanks a lot!
>
> Link to updated webrevs -
> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13/
> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13_to_12/
Thank you for fixing all.
Looks good to me except below.

Could you update the copyright format in TestAllocateHeapAt.java?
2  * Copyright (c) 2017 Oracle and/or its affiliates. All rights reserved.
- Missing comma: * Copyright (c) 2017_*,*_ Oracle and/or its affiliates.
All rights reserved.

Thanks,
Sangheon


>
> @Sangheon: Please let me know if you see any corrections needed.
>
> -Kishor
>
>> -----Original Message-----
>> From: Thomas Schatzl [mailto:[hidden email]]
>> Sent: Friday, November 3, 2017 7:31 AM
>> To: Kharbas, Kishor <[hidden email]>; sangheon.kim
>> <[hidden email]>; '[hidden email]'
>> <[hidden email]>; hotspot-runtime-
>> [hidden email]
>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on alternative
>> memory devices and CSR review
>>
>> Hi,
>>
>> On Fri, 2017-11-03 at 08:55 +0000, Kharbas, Kishor wrote:
>>> 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 minor nits:
>>
>>   - os_posix.cpp:300: please move the else next to the brace
>>   - arguments.cpp:4624: please add a space between "if" and the bracket
>>
>> I do not need to see a new webrev for these changes. Looks good.
>>
>> Thanks,
>>    Thomas

Reply | Threaded
Open this post in threaded view
|

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

Doerr, Martin
Hi,

I just noticed that "MAP_NORESERVE" is not defined on AIX (os_posix.cpp reserve_mmapped_memory).
I guess it's not POSIX, so this looks like a bug.
Would it make sense to replace it by "NOT_AIX( | MAP_NORESERVE )"? Or is there a better idea?

Best regards,
Martin


-----Original Message-----
From: hotspot-gc-dev [mailto:[hidden email]] On Behalf Of Kharbas, Kishor
Sent: Montag, 13. November 2017 22:33
To: sangheon.kim <[hidden email]>; Thomas Schatzl <[hidden email]>; '[hidden email]' <[hidden email]>; [hidden email]
Cc: Kharbas, Kishor <[hidden email]>
Subject: RE: RFR(M): 8190308: Supporting heap allocation on alternative memory devices and CSR review

Thanks Sangheon and Thomas!

-----Original Message-----
From: sangheon.kim [mailto:[hidden email]]
Sent: Monday, November 13, 2017 12:41 PM
To: Thomas Schatzl <[hidden email]>; Kharbas, Kishor <[hidden email]>; '[hidden email]' <[hidden email]>; [hidden email]
Subject: Re: RFR(M): 8190308: Supporting heap allocation on alternative memory devices and CSR review

Hi Kishor,

On 11/13/2017 12:40 PM, Thomas Schatzl wrote:

> Hi Kishor,
>
> On Mon, 2017-11-13 at 19:40 +0000, Kharbas, Kishor wrote:
>> Greetings,
>>  
>> I have an updated webrev to remove compilation warning on Windows 32-
>> bit.
>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15/
>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15_to_14/
>>  
>> Sorry missed this earlier. I request for a review on this update.
>    looks good. The other changes from webrev.13 on also look good.
+1

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>
>>  
>> Thanks
>> Kishor
>>  
>> From: sangheon.kim [mailto:[hidden email]]
>> Sent: Friday, November 3, 2017 4:07 PM
>> To: Kharbas, Kishor <[hidden email]>; Thomas Schatzl <thoma
>> [hidden email]>; '[hidden email]' <hotspot-gc-
>> [hidden email]>; [hidden email]
>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>> alternative memory devices and CSR review
>>  
>> Hi Kishor,
>>
>>
>> On 11/03/2017 02:59 PM, Kharbas, Kishor wrote:
>> Hi Sangheon,
>>  
>> Here is link to the updated webrev-
>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.14/
>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.14_to_13/
>> Looks good to me.
>>
>> Thanks,
>> Sangheon
>>
>>
>>
>>  
>> Thanks
>> Kishor
>>  
>> From: sangheon.kim [mailto:[hidden email]]
>> Sent: Friday, November 3, 2017 2:38 PM
>> To: Kharbas, Kishor <[hidden email]>; Thomas Schatzl <thoma
>> [hidden email]>; '[hidden email]' <hotspot-gc-
>> [hidden email]>; [hidden email]
>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>> alternative memory devices and CSR review
>>  
>> Hi Kishor,
>>
>> On 11/03/2017 11:39 AM, Kharbas, Kishor wrote:
>> Thanks a lot!
>>  
>> Link to updated webrevs -
>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13/
>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13_to_12/
>> Thank you for fixing all.
>> Looks good to me except below.
>>
>> Could you update the copyright format in TestAllocateHeapAt.java?
>> 2  * Copyright (c) 2017 Oracle and/or its affiliates. All rights
>> reserved.
>> - Missing comma: * Copyright (c) 2017, Oracle and/or its affiliates.
>> All rights reserved.
>>
>> Thanks,
>> Sangheon
>>
>>
>>
>>
>>  
>>  
>> @Sangheon: Please let me know if you see any corrections needed.
>>  
>> -Kishor
>>  
>> -----Original Message-----
>> From: Thomas Schatzl [mailto:[hidden email]]
>> Sent: Friday, November 3, 2017 7:31 AM
>> To: Kharbas, Kishor <[hidden email]>; sangheon.kim
>> <[hidden email]>; '[hidden email]'
>> <[hidden email]>; hotspot-runtime-
>> [hidden email]
>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>> alternative memory devices and CSR review
>>  
>> Hi,
>>  
>> On Fri, 2017-11-03 at 08:55 +0000, Kharbas, Kishor wrote:
>> 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 minor nits:
>>  
>>   - os_posix.cpp:300: please move the else next to the brace
>>   - arguments.cpp:4624: please add a space between "if" and the
>> bracket
>>  
>> I do not need to see a new webrev for these changes. Looks good.
>>  
>> Thanks,
>>    Thomas
>>  
>>  
>>  

Reply | Threaded
Open this post in threaded view
|

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

sangheon.kim@oracle.com
Hi Martin,

On 11/30/2017 10:08 AM, Doerr, Martin wrote:
> Hi,
>
> I just noticed that "MAP_NORESERVE" is not defined on AIX (os_posix.cpp reserve_mmapped_memory).
> I guess it's not POSIX, so this looks like a bug.
Yes, I agree.
I filed https://bugs.openjdk.java.net/browse/JDK-8192919 for this.

> Would it make sense to replace it by "NOT_AIX( | MAP_NORESERVE )"?
That would be a good quick-fix for AIX.

But MAP_ANONYMOUS (used with MAP_NORESERVE in that line) is also not
specified on POSIX as well. The difference is that most OS seem to
define it. Or line 56(#ifndef MAP_ANONYMOUS) makes to avoid the problem.
So I think it would be better to completely move these 2 flags out of
os_posix.cpp.

Thanks,
Sangheon


>   Or is there a better idea?
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: hotspot-gc-dev [mailto:[hidden email]] On Behalf Of Kharbas, Kishor
> Sent: Montag, 13. November 2017 22:33
> To: sangheon.kim <[hidden email]>; Thomas Schatzl <[hidden email]>; '[hidden email]' <[hidden email]>; [hidden email]
> Cc: Kharbas, Kishor <[hidden email]>
> Subject: RE: RFR(M): 8190308: Supporting heap allocation on alternative memory devices and CSR review
>
> Thanks Sangheon and Thomas!
>
> -----Original Message-----
> From: sangheon.kim [mailto:[hidden email]]
> Sent: Monday, November 13, 2017 12:41 PM
> To: Thomas Schatzl <[hidden email]>; Kharbas, Kishor <[hidden email]>; '[hidden email]' <[hidden email]>; [hidden email]
> Subject: Re: RFR(M): 8190308: Supporting heap allocation on alternative memory devices and CSR review
>
> Hi Kishor,
>
> On 11/13/2017 12:40 PM, Thomas Schatzl wrote:
>> Hi Kishor,
>>
>> On Mon, 2017-11-13 at 19:40 +0000, Kharbas, Kishor wrote:
>>> Greetings,
>>>    
>>> I have an updated webrev to remove compilation warning on Windows 32-
>>> bit.
>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15/
>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15_to_14/
>>>    
>>> Sorry missed this earlier. I request for a review on this update.
>>     looks good. The other changes from webrev.13 on also look good.
> +1
>
> Thanks,
> Sangheon
>
>
>> Thanks,
>>     Thomas
>>
>>>    
>>> Thanks
>>> Kishor
>>>    
>>> From: sangheon.kim [mailto:[hidden email]]
>>> Sent: Friday, November 3, 2017 4:07 PM
>>> To: Kharbas, Kishor <[hidden email]>; Thomas Schatzl <thoma
>>> [hidden email]>; '[hidden email]' <hotspot-gc-
>>> [hidden email]>; [hidden email]
>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>> alternative memory devices and CSR review
>>>    
>>> Hi Kishor,
>>>
>>>
>>> On 11/03/2017 02:59 PM, Kharbas, Kishor wrote:
>>> Hi Sangheon,
>>>    
>>> Here is link to the updated webrev-
>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.14/
>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.14_to_13/
>>> Looks good to me.
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>
>>>    
>>> Thanks
>>> Kishor
>>>    
>>> From: sangheon.kim [mailto:[hidden email]]
>>> Sent: Friday, November 3, 2017 2:38 PM
>>> To: Kharbas, Kishor <[hidden email]>; Thomas Schatzl <thoma
>>> [hidden email]>; '[hidden email]' <hotspot-gc-
>>> [hidden email]>; [hidden email]
>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>> alternative memory devices and CSR review
>>>    
>>> Hi Kishor,
>>>
>>> On 11/03/2017 11:39 AM, Kharbas, Kishor wrote:
>>> Thanks a lot!
>>>    
>>> Link to updated webrevs -
>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13/
>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13_to_12/
>>> Thank you for fixing all.
>>> Looks good to me except below.
>>>
>>> Could you update the copyright format in TestAllocateHeapAt.java?
>>> 2  * Copyright (c) 2017 Oracle and/or its affiliates. All rights
>>> reserved.
>>> - Missing comma: * Copyright (c) 2017, Oracle and/or its affiliates.
>>> All rights reserved.
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>
>>>
>>>    
>>>    
>>> @Sangheon: Please let me know if you see any corrections needed.
>>>    
>>> -Kishor
>>>    
>>> -----Original Message-----
>>> From: Thomas Schatzl [mailto:[hidden email]]
>>> Sent: Friday, November 3, 2017 7:31 AM
>>> To: Kharbas, Kishor <[hidden email]>; sangheon.kim
>>> <[hidden email]>; '[hidden email]'
>>> <[hidden email]>; hotspot-runtime-
>>> [hidden email]
>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>> alternative memory devices and CSR review
>>>    
>>> Hi,
>>>    
>>> On Fri, 2017-11-03 at 08:55 +0000, Kharbas, Kishor wrote:
>>> 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 minor nits:
>>>    
>>>    - os_posix.cpp:300: please move the else next to the brace
>>>    - arguments.cpp:4624: please add a space between "if" and the
>>> bracket
>>>    
>>> I do not need to see a new webrev for these changes. Looks good.
>>>    
>>> Thanks,
>>>     Thomas
>>>    
>>>    
>>>    

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 2/12/2017 4:59 AM, sangheon.kim wrote:

> Hi Martin,
>
> On 11/30/2017 10:08 AM, Doerr, Martin wrote:
>> Hi,
>>
>> I just noticed that "MAP_NORESERVE" is not defined on AIX
>> (os_posix.cpp reserve_mmapped_memory).
>> I guess it's not POSIX, so this looks like a bug.
> Yes, I agree.
> I filed https://bugs.openjdk.java.net/browse/JDK-8192919 for this.
>
>> Would it make sense to replace it by "NOT_AIX( | MAP_NORESERVE )"?
> That would be a good quick-fix for AIX.
>
> But MAP_ANONYMOUS (used with MAP_NORESERVE in that line) is also not
> specified on POSIX as well. The difference is that most OS seem to
> define it. Or line 56(#ifndef MAP_ANONYMOUS) makes to avoid the problem.
> So I think it would be better to completely move these 2 flags out of
> os_posix.cpp.

Moved out or only used for each OS that has established they are correct
for that OS. I've updated the bug with a request to re-open it.

Thanks,
David

> Thanks,
> Sangheon
>
>
>>   Or is there a better idea?
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: hotspot-gc-dev [mailto:[hidden email]]
>> On Behalf Of Kharbas, Kishor
>> Sent: Montag, 13. November 2017 22:33
>> To: sangheon.kim <[hidden email]>; Thomas Schatzl
>> <[hidden email]>; '[hidden email]'
>> <[hidden email]>; [hidden email]
>> Cc: Kharbas, Kishor <[hidden email]>
>> Subject: RE: RFR(M): 8190308: Supporting heap allocation on
>> alternative memory devices and CSR review
>>
>> Thanks Sangheon and Thomas!
>>
>> -----Original Message-----
>> From: sangheon.kim [mailto:[hidden email]]
>> Sent: Monday, November 13, 2017 12:41 PM
>> To: Thomas Schatzl <[hidden email]>; Kharbas, Kishor
>> <[hidden email]>; '[hidden email]'
>> <[hidden email]>; [hidden email]
>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>> alternative memory devices and CSR review
>>
>> Hi Kishor,
>>
>> On 11/13/2017 12:40 PM, Thomas Schatzl wrote:
>>> Hi Kishor,
>>>
>>> On Mon, 2017-11-13 at 19:40 +0000, Kharbas, Kishor wrote:
>>>> Greetings,
>>>> I have an updated webrev to remove compilation warning on Windows 32-
>>>> bit.
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15/
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15_to_14/
>>>> Sorry missed this earlier. I request for a review on this update.
>>>     looks good. The other changes from webrev.13 on also look good.
>> +1
>>
>> Thanks,
>> Sangheon
>>
>>
>>> Thanks,
>>>     Thomas
>>>
>>>> Thanks
>>>> Kishor
>>>> From: sangheon.kim [mailto:[hidden email]]
>>>> Sent: Friday, November 3, 2017 4:07 PM
>>>> To: Kharbas, Kishor <[hidden email]>; Thomas Schatzl <thoma
>>>> [hidden email]>; '[hidden email]' <hotspot-gc-
>>>> [hidden email]>; [hidden email]
>>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>>> alternative memory devices and CSR review
>>>> Hi Kishor,
>>>>
>>>>
>>>> On 11/03/2017 02:59 PM, Kharbas, Kishor wrote:
>>>> Hi Sangheon,
>>>> Here is link to the updated webrev-
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.14/
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.14_to_13/
>>>> Looks good to me.
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>
>>>> Thanks
>>>> Kishor
>>>> From: sangheon.kim [mailto:[hidden email]]
>>>> Sent: Friday, November 3, 2017 2:38 PM
>>>> To: Kharbas, Kishor <[hidden email]>; Thomas Schatzl <thoma
>>>> [hidden email]>; '[hidden email]' <hotspot-gc-
>>>> [hidden email]>; [hidden email]
>>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>>> alternative memory devices and CSR review
>>>> Hi Kishor,
>>>>
>>>> On 11/03/2017 11:39 AM, Kharbas, Kishor wrote:
>>>> Thanks a lot!
>>>> Link to updated webrevs -
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13/
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13_to_12/
>>>> Thank you for fixing all.
>>>> Looks good to me except below.
>>>>
>>>> Could you update the copyright format in TestAllocateHeapAt.java?
>>>> 2  * Copyright (c) 2017 Oracle and/or its affiliates. All rights
>>>> reserved.
>>>> - Missing comma: * Copyright (c) 2017, Oracle and/or its affiliates.
>>>> All rights reserved.
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>
>>>>
>>>> @Sangheon: Please let me know if you see any corrections needed.
>>>> -Kishor
>>>> -----Original Message-----
>>>> From: Thomas Schatzl [mailto:[hidden email]]
>>>> Sent: Friday, November 3, 2017 7:31 AM
>>>> To: Kharbas, Kishor <[hidden email]>; sangheon.kim
>>>> <[hidden email]>; '[hidden email]'
>>>> <[hidden email]>; hotspot-runtime-
>>>> [hidden email]
>>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>>> alternative memory devices and CSR review
>>>> Hi,
>>>> On Fri, 2017-11-03 at 08:55 +0000, Kharbas, Kishor wrote:
>>>> 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 minor nits:
>>>>    - os_posix.cpp:300: please move the else next to the brace
>>>>    - arguments.cpp:4624: please add a space between "if" and the
>>>> bracket
>>>> I do not need to see a new webrev for these changes. Looks good.
>>>> Thanks,
>>>>     Thomas
>
Reply | Threaded
Open this post in threaded view
|

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

Doerr, Martin
Hi Sangheon and David,

first of all, sorry that I forgot to mention JDK-8192898 "AIX build broken after JDK-8190308" in this email thread. It was a quick fix to get AIX working again for the jdk10 fork.

Thanks for opening the new bug JDK-8192919 and for sharing your thoughts. Sounds like a nice cleanup for jdk11.

Best regards,
Martin


-----Original Message-----
From: David Holmes [mailto:[hidden email]]
Sent: Sonntag, 3. Dezember 2017 03:51
To: sangheon.kim <[hidden email]>; Doerr, Martin <[hidden email]>; Kharbas, Kishor <[hidden email]>; Thomas Schatzl <[hidden email]>; '[hidden email]' <[hidden email]>; [hidden email]; Stuefe, Thomas <[hidden email]>
Subject: Re: RFR(M): 8190308: Supporting heap allocation on alternative memory devices and CSR review

On 2/12/2017 4:59 AM, sangheon.kim wrote:

> Hi Martin,
>
> On 11/30/2017 10:08 AM, Doerr, Martin wrote:
>> Hi,
>>
>> I just noticed that "MAP_NORESERVE" is not defined on AIX
>> (os_posix.cpp reserve_mmapped_memory).
>> I guess it's not POSIX, so this looks like a bug.
> Yes, I agree.
> I filed https://bugs.openjdk.java.net/browse/JDK-8192919 for this.
>
>> Would it make sense to replace it by "NOT_AIX( | MAP_NORESERVE )"?
> That would be a good quick-fix for AIX.
>
> But MAP_ANONYMOUS (used with MAP_NORESERVE in that line) is also not
> specified on POSIX as well. The difference is that most OS seem to
> define it. Or line 56(#ifndef MAP_ANONYMOUS) makes to avoid the problem.
> So I think it would be better to completely move these 2 flags out of
> os_posix.cpp.

Moved out or only used for each OS that has established they are correct
for that OS. I've updated the bug with a request to re-open it.

Thanks,
David

> Thanks,
> Sangheon
>
>
>>   Or is there a better idea?
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: hotspot-gc-dev [mailto:[hidden email]]
>> On Behalf Of Kharbas, Kishor
>> Sent: Montag, 13. November 2017 22:33
>> To: sangheon.kim <[hidden email]>; Thomas Schatzl
>> <[hidden email]>; '[hidden email]'
>> <[hidden email]>; [hidden email]
>> Cc: Kharbas, Kishor <[hidden email]>
>> Subject: RE: RFR(M): 8190308: Supporting heap allocation on
>> alternative memory devices and CSR review
>>
>> Thanks Sangheon and Thomas!
>>
>> -----Original Message-----
>> From: sangheon.kim [mailto:[hidden email]]
>> Sent: Monday, November 13, 2017 12:41 PM
>> To: Thomas Schatzl <[hidden email]>; Kharbas, Kishor
>> <[hidden email]>; '[hidden email]'
>> <[hidden email]>; [hidden email]
>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>> alternative memory devices and CSR review
>>
>> Hi Kishor,
>>
>> On 11/13/2017 12:40 PM, Thomas Schatzl wrote:
>>> Hi Kishor,
>>>
>>> On Mon, 2017-11-13 at 19:40 +0000, Kharbas, Kishor wrote:
>>>> Greetings,
>>>> I have an updated webrev to remove compilation warning on Windows 32-
>>>> bit.
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15/
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15_to_14/
>>>> Sorry missed this earlier. I request for a review on this update.
>>>     looks good. The other changes from webrev.13 on also look good.
>> +1
>>
>> Thanks,
>> Sangheon
>>
>>
>>> Thanks,
>>>     Thomas
>>>
>>>> Thanks
>>>> Kishor
>>>> From: sangheon.kim [mailto:[hidden email]]
>>>> Sent: Friday, November 3, 2017 4:07 PM
>>>> To: Kharbas, Kishor <[hidden email]>; Thomas Schatzl <thoma
>>>> [hidden email]>; '[hidden email]' <hotspot-gc-
>>>> [hidden email]>; [hidden email]
>>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>>> alternative memory devices and CSR review
>>>> Hi Kishor,
>>>>
>>>>
>>>> On 11/03/2017 02:59 PM, Kharbas, Kishor wrote:
>>>> Hi Sangheon,
>>>> Here is link to the updated webrev-
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.14/
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.14_to_13/
>>>> Looks good to me.
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>
>>>> Thanks
>>>> Kishor
>>>> From: sangheon.kim [mailto:[hidden email]]
>>>> Sent: Friday, November 3, 2017 2:38 PM
>>>> To: Kharbas, Kishor <[hidden email]>; Thomas Schatzl <thoma
>>>> [hidden email]>; '[hidden email]' <hotspot-gc-
>>>> [hidden email]>; [hidden email]
>>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>>> alternative memory devices and CSR review
>>>> Hi Kishor,
>>>>
>>>> On 11/03/2017 11:39 AM, Kharbas, Kishor wrote:
>>>> Thanks a lot!
>>>> Link to updated webrevs -
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13/
>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13_to_12/
>>>> Thank you for fixing all.
>>>> Looks good to me except below.
>>>>
>>>> Could you update the copyright format in TestAllocateHeapAt.java?
>>>> 2  * Copyright (c) 2017 Oracle and/or its affiliates. All rights
>>>> reserved.
>>>> - Missing comma: * Copyright (c) 2017, Oracle and/or its affiliates.
>>>> All rights reserved.
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>
>>>>
>>>> @Sangheon: Please let me know if you see any corrections needed.
>>>> -Kishor
>>>> -----Original Message-----
>>>> From: Thomas Schatzl [mailto:[hidden email]]
>>>> Sent: Friday, November 3, 2017 7:31 AM
>>>> To: Kharbas, Kishor <[hidden email]>; sangheon.kim
>>>> <[hidden email]>; '[hidden email]'
>>>> <[hidden email]>; hotspot-runtime-
>>>> [hidden email]
>>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>>> alternative memory devices and CSR review
>>>> Hi,
>>>> On Fri, 2017-11-03 at 08:55 +0000, Kharbas, Kishor wrote:
>>>> 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 minor nits:
>>>>    - os_posix.cpp:300: please move the else next to the brace
>>>>    - arguments.cpp:4624: please add a space between "if" and the
>>>> bracket
>>>> I do not need to see a new webrev for these changes. Looks good.
>>>> Thanks,
>>>>     Thomas
>
Reply | Threaded
Open this post in threaded view
|

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

sangheon.kim@oracle.com
Hi David and Martin,

I re-opened JDK-8192919 with a new title.
This enhancement is to remove non-portable flags used in os_posix.cpp.

Thanks,
Sangheon


On 12/04/2017 02:27 AM, Doerr, Martin wrote:

> Hi Sangheon and David,
>
> first of all, sorry that I forgot to mention JDK-8192898 "AIX build broken after JDK-8190308" in this email thread. It was a quick fix to get AIX working again for the jdk10 fork.
>
> Thanks for opening the new bug JDK-8192919 and for sharing your thoughts. Sounds like a nice cleanup for jdk11.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: David Holmes [mailto:[hidden email]]
> Sent: Sonntag, 3. Dezember 2017 03:51
> To: sangheon.kim <[hidden email]>; Doerr, Martin <[hidden email]>; Kharbas, Kishor <[hidden email]>; Thomas Schatzl <[hidden email]>; '[hidden email]' <[hidden email]>; [hidden email]; Stuefe, Thomas <[hidden email]>
> Subject: Re: RFR(M): 8190308: Supporting heap allocation on alternative memory devices and CSR review
>
> On 2/12/2017 4:59 AM, sangheon.kim wrote:
>> Hi Martin,
>>
>> On 11/30/2017 10:08 AM, Doerr, Martin wrote:
>>> Hi,
>>>
>>> I just noticed that "MAP_NORESERVE" is not defined on AIX
>>> (os_posix.cpp reserve_mmapped_memory).
>>> I guess it's not POSIX, so this looks like a bug.
>> Yes, I agree.
>> I filed https://bugs.openjdk.java.net/browse/JDK-8192919 for this.
>>
>>> Would it make sense to replace it by "NOT_AIX( | MAP_NORESERVE )"?
>> That would be a good quick-fix for AIX.
>>
>> But MAP_ANONYMOUS (used with MAP_NORESERVE in that line) is also not
>> specified on POSIX as well. The difference is that most OS seem to
>> define it. Or line 56(#ifndef MAP_ANONYMOUS) makes to avoid the problem.
>> So I think it would be better to completely move these 2 flags out of
>> os_posix.cpp.
> Moved out or only used for each OS that has established they are correct
> for that OS. I've updated the bug with a request to re-open it.
>
> Thanks,
> David
>
>> Thanks,
>> Sangheon
>>
>>
>>>    Or is there a better idea?
>>>
>>> Best regards,
>>> Martin
>>>
>>>
>>> -----Original Message-----
>>> From: hotspot-gc-dev [mailto:[hidden email]]
>>> On Behalf Of Kharbas, Kishor
>>> Sent: Montag, 13. November 2017 22:33
>>> To: sangheon.kim <[hidden email]>; Thomas Schatzl
>>> <[hidden email]>; '[hidden email]'
>>> <[hidden email]>; [hidden email]
>>> Cc: Kharbas, Kishor <[hidden email]>
>>> Subject: RE: RFR(M): 8190308: Supporting heap allocation on
>>> alternative memory devices and CSR review
>>>
>>> Thanks Sangheon and Thomas!
>>>
>>> -----Original Message-----
>>> From: sangheon.kim [mailto:[hidden email]]
>>> Sent: Monday, November 13, 2017 12:41 PM
>>> To: Thomas Schatzl <[hidden email]>; Kharbas, Kishor
>>> <[hidden email]>; '[hidden email]'
>>> <[hidden email]>; [hidden email]
>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>> alternative memory devices and CSR review
>>>
>>> Hi Kishor,
>>>
>>> On 11/13/2017 12:40 PM, Thomas Schatzl wrote:
>>>> Hi Kishor,
>>>>
>>>> On Mon, 2017-11-13 at 19:40 +0000, Kharbas, Kishor wrote:
>>>>> Greetings,
>>>>> I have an updated webrev to remove compilation warning on Windows 32-
>>>>> bit.
>>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15/
>>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.15_to_14/
>>>>> Sorry missed this earlier. I request for a review on this update.
>>>>      looks good. The other changes from webrev.13 on also look good.
>>> +1
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>> Thanks,
>>>>      Thomas
>>>>
>>>>> Thanks
>>>>> Kishor
>>>>> From: sangheon.kim [mailto:[hidden email]]
>>>>> Sent: Friday, November 3, 2017 4:07 PM
>>>>> To: Kharbas, Kishor <[hidden email]>; Thomas Schatzl <thoma
>>>>> [hidden email]>; '[hidden email]' <hotspot-gc-
>>>>> [hidden email]>; [hidden email]
>>>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>>>> alternative memory devices and CSR review
>>>>> Hi Kishor,
>>>>>
>>>>>
>>>>> On 11/03/2017 02:59 PM, Kharbas, Kishor wrote:
>>>>> Hi Sangheon,
>>>>> Here is link to the updated webrev-
>>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.14/
>>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.14_to_13/
>>>>> Looks good to me.
>>>>>
>>>>> Thanks,
>>>>> Sangheon
>>>>>
>>>>>
>>>>>
>>>>> Thanks
>>>>> Kishor
>>>>> From: sangheon.kim [mailto:[hidden email]]
>>>>> Sent: Friday, November 3, 2017 2:38 PM
>>>>> To: Kharbas, Kishor <[hidden email]>; Thomas Schatzl <thoma
>>>>> [hidden email]>; '[hidden email]' <hotspot-gc-
>>>>> [hidden email]>; [hidden email]
>>>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>>>> alternative memory devices and CSR review
>>>>> Hi Kishor,
>>>>>
>>>>> On 11/03/2017 11:39 AM, Kharbas, Kishor wrote:
>>>>> Thanks a lot!
>>>>> Link to updated webrevs -
>>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13/
>>>>> http://cr.openjdk.java.net/~kkharbas/8190308/webrev.13_to_12/
>>>>> Thank you for fixing all.
>>>>> Looks good to me except below.
>>>>>
>>>>> Could you update the copyright format in TestAllocateHeapAt.java?
>>>>> 2  * Copyright (c) 2017 Oracle and/or its affiliates. All rights
>>>>> reserved.
>>>>> - Missing comma: * Copyright (c) 2017, Oracle and/or its affiliates.
>>>>> All rights reserved.
>>>>>
>>>>> Thanks,
>>>>> Sangheon
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> @Sangheon: Please let me know if you see any corrections needed.
>>>>> -Kishor
>>>>> -----Original Message-----
>>>>> From: Thomas Schatzl [mailto:[hidden email]]
>>>>> Sent: Friday, November 3, 2017 7:31 AM
>>>>> To: Kharbas, Kishor <[hidden email]>; sangheon.kim
>>>>> <[hidden email]>; '[hidden email]'
>>>>> <[hidden email]>; hotspot-runtime-
>>>>> [hidden email]
>>>>> Subject: Re: RFR(M): 8190308: Supporting heap allocation on
>>>>> alternative memory devices and CSR review
>>>>> Hi,
>>>>> On Fri, 2017-11-03 at 08:55 +0000, Kharbas, Kishor wrote:
>>>>> 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 minor nits:
>>>>>     - os_posix.cpp:300: please move the else next to the brace
>>>>>     - arguments.cpp:4624: please add a space between "if" and the
>>>>> bracket
>>>>> I do not need to see a new webrev for these changes. Looks good.
>>>>> Thanks,
>>>>>      Thomas