<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Kishor,<br>
<br>
<div class="moz-cite-prefix">On 10/3/18 6:24 PM, Kharbas, Kishor
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3BEC4891A@ORSMSX116.amr.corp.intel.com">
<pre wrap="">Hi,
I have an update to the webrev which addresses comments from Sangheon during an offline discussion.
The JEP has been moved to an enhancement, consequently bug id '8204908' has been closed. I have created a new issue for this implementation - <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8211424">https://bugs.openjdk.java.net/browse/JDK-8211424</a>.
Check the comments in main issue - <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8202286">https://bugs.openjdk.java.net/browse/JDK-8202286</a>.
Any suggestion on whether I continue on this thread or start new one. Not only the bug id has changed, but also the design, implementation.</pre>
</blockquote>
Thanks for the explanation.<br>
<br>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3BEC4891A@ORSMSX116.amr.corp.intel.com">
<pre wrap="">
Full: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekkharbas/8211424/webrev-8204908.03">http://cr.openjdk.java.net/~kkharbas/8211424/webrev-8204908.03</a>
Incremental : <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekkharbas/8211424/webrev-8204908.03_to_02">http://cr.openjdk.java.net/~kkharbas/8211424/webrev-8204908.03_to_02</a></pre>
</blockquote>
webrev.03 looks okay in general.<br>
A few minor nits:<br>
<br>
src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.cpp<br>
104
_max_old_byte_size(_max_total_size - _min_young_byte_size), <br>
105
_max_young_byte_size(_max_total_size - _min_old_byte_size), <br>
106
_max_total_size(max_total_size) {<br>
- line 104 and 105 will use uninitialized value of _max_total_size.
I couldn't catch it before.<br>
<br>
<br>
One style comment is that adjoiningGenerationsForHeteroHeap.cpp uses
both high()/low() and young_vs()/old_vs() which makes me confused. I
would prefer not to add young_vs()/old_vs(), but I don't have strong
opinion on this. But using only one(high() or young_vs()) instead of
mixed use of high() or young_vs() at ctor seems better.<br>
<br>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3BEC4891A@ORSMSX116.amr.corp.intel.com">
<pre wrap="">
Testing : Passed tier1_gc and tier1_runtime jtret tests.
I added extra options "-XX:+UnlockExperimentalVMOptions -XX:AllocateOldGenAt=/home/Kishor" to each test. There are some tests which I had to exclude since they won't work with this flag. Example: tests in ConcMarkSweepGC which does not support this flag, tests requiring CompressedOops to be enabled, etc.
</pre>
</blockquote>
Thanks for testing. <br>
Could you also test without AllocateOldGetAt option enabled? As this
option will be disabled as a default, modified codes also need
verifications.<br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3BEC4891A@ORSMSX116.amr.corp.intel.com">
<pre wrap="">
Thanks,
Kishor
</pre>
<blockquote type="cite">
<pre wrap="">-----Original Message-----
From: Kharbas, Kishor
Sent: Wednesday, September 19, 2018 3:57 PM
To: <a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a>; Thomas Stüfe
<a class="moz-txt-link-rfc2396E" href="mailto:thomas.stuefe@gmail.com"><thomas.stuefe@gmail.com></a>; Thomas Schatzl
<a class="moz-txt-link-rfc2396E" href="mailto:thomas.schatzl@oracle.com"><thomas.schatzl@oracle.com></a>
Cc: <a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>; Hotspot dev runtime <hotspot-
<a class="moz-txt-link-abbreviated" href="mailto:runtime-dev@openjdk.java.net">runtime-dev@openjdk.java.net</a>>; Viswanathan, Sandhya
<a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>; Aundhe, Shirish
<a class="moz-txt-link-rfc2396E" href="mailto:shirish.aundhe@intel.com"><shirish.aundhe@intel.com></a>; Kharbas, Kishor <a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>
Subject: RE: RFR(M): 8204908: NVDIMM for POGC and G1GC -
ReserveSpace.cpp changes are mostly eliminated/no collector specific code.
Hi,
I have an small update to the patch to disable UseCompressedOops.
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekkharbas/8204908/webrev-8204908.02/">http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.02/</a>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekkharbas/8204908/webrev-8204908.02_to_01/">http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.02_to_01/</a>
Regards,
Kishor
</pre>
<blockquote type="cite">
<pre wrap="">-----Original Message-----
From: Kharbas, Kishor
Sent: Wednesday, September 19, 2018 9:35 AM
To: '<a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a>' <a class="moz-txt-link-rfc2396E" href="mailto:sangheon.kim@oracle.com"><sangheon.kim@oracle.com></a>; Thomas
</pre>
</blockquote>
<pre wrap="">Stüfe
</pre>
<blockquote type="cite">
<pre wrap=""><a class="moz-txt-link-rfc2396E" href="mailto:thomas.stuefe@gmail.com"><thomas.stuefe@gmail.com></a>; Thomas Schatzl
</pre>
</blockquote>
<pre wrap=""><a class="moz-txt-link-rfc2396E" href="mailto:thomas.schatzl@oracle.com"><thomas.schatzl@oracle.com></a>
</pre>
<blockquote type="cite">
<pre wrap="">Cc: <a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>; Hotspot dev runtime <hotspot-
<a class="moz-txt-link-abbreviated" href="mailto:runtime-dev@openjdk.java.net">runtime-dev@openjdk.java.net</a>>; Viswanathan, Sandhya
<a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>; Aundhe, Shirish
<a class="moz-txt-link-rfc2396E" href="mailto:shirish.aundhe@intel.com"><shirish.aundhe@intel.com></a>; Kharbas, Kishor <a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>
Subject: RE: RFR(M): 8204908: NVDIMM for POGC and G1GC -
ReserveSpace.cpp changes are mostly eliminated/no collector specific
</pre>
</blockquote>
<pre wrap="">code.
</pre>
<blockquote type="cite">
<pre wrap="">Thanks Sangheon,
I have uploaded the updated patch at,
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekkharbas/8204908/webrev-8204908.01/">http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.01/</a>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekkharbas/8204908/webrev">http://cr.openjdk.java.net/~kkharbas/8204908/webrev</a>-
</pre>
</blockquote>
<pre wrap="">8204908.01_to_00/
</pre>
<blockquote type="cite">
<pre wrap="">I have fixed all the indentation and format related comments, others I
have pasted here below with my comments inline.
=============================
</pre>
<blockquote type="cite">
<pre wrap="">src/hotspot/share/gc/parallel/adjoiningGenerations.hpp
- Copyright update
62 AdjoiningGenerations();
- Why we need this ctor?
</pre>
<blockquote type="cite">
<pre wrap="">I need this default ctor for constructing
</pre>
</blockquote>
</blockquote>
<pre wrap="">adjoiningGenerationsForHeteroHeap, since I don't want to call the non-
default constructor of adjoiningGenerations.
</pre>
<blockquote type="cite">
<pre wrap="">=========================
/src/hotspot/share/gc/parallel/adjoiningVirtualSpaces.hpp
- Copyright update
88 virtual PSVirtualSpace* high() { return _high; }
89 virtual PSVirtualSpace* low() { return _low; }
- Those methods don't need 'virtual' specifier as high()/low()
methods are only used at ctor of AdjoiningGenerations. The other
calling site is ctor of AdjoiningGenerationsForHeteroHeap but it is
used another type of
high()/low() which returns _young_vs or _old_vs.
</pre>
<blockquote type="cite">
<pre wrap="">I feel overriding these methods is a good idea from design
standpoint;
</pre>
</blockquote>
</blockquote>
<pre wrap="">code changes in future would take benefit of this.
========================
</pre>
<blockquote type="cite">
<pre wrap="">src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.hpp
45 PSVirtualSpace* _young_vs;
46 PSVirtualSpace* _old_vs;
- Can't we use 'AdjoiningVirtualSpaces::_low' and '_high' instead?
If it is not the case, adding high(),low() may result in confusion
between
AdjoiningVirtualSpaces::high() and low(). So use other name for
current HeteroVirtualSpaces::high()/low().
</pre>
<blockquote type="cite">
<pre wrap="">AdjoiningVirtualSpaces contains two adjacent virtual spaces in the
same
</pre>
</blockquote>
</blockquote>
<pre wrap="">reserved space and separated by a boundary. That’s why the name 'high'
and 'low'.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">The class I added - HeteroVirtualSpaces, are not adjacent and do
not
</pre>
</blockquote>
</blockquote>
<pre wrap="">share same reserved space. So I have names them '_young_vs' and
</pre>
</blockquote>
<pre wrap="">'_old_vs'.
</pre>
<blockquote type="cite">
<pre wrap="">But from outside, i.e, users of VirtualSpaces, they call high() and
low() to access these spaces. So I have not changed the function names.
</pre>
<blockquote type="cite">
<pre wrap="">-----Original Message-----
From: <a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a> [<a class="moz-txt-link-freetext" href="mailto:sangheon.kim@oracle.com">mailto:sangheon.kim@oracle.com</a>]
Sent: Monday, September 17, 2018 11:32 AM
To: Kharbas, Kishor <a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>; Thomas Stüfe
<a class="moz-txt-link-rfc2396E" href="mailto:thomas.stuefe@gmail.com"><thomas.stuefe@gmail.com></a>; Thomas Schatzl
</pre>
</blockquote>
<pre wrap=""><a class="moz-txt-link-rfc2396E" href="mailto:thomas.schatzl@oracle.com"><thomas.schatzl@oracle.com></a>
</pre>
<blockquote type="cite">
<pre wrap="">Cc: <a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>; Hotspot dev runtime <hotspot-
<a class="moz-txt-link-abbreviated" href="mailto:runtime-dev@openjdk.java.net">runtime-dev@openjdk.java.net</a>>; Viswanathan, Sandhya
<a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>; Aundhe, Shirish
<a class="moz-txt-link-rfc2396E" href="mailto:shirish.aundhe@intel.com"><shirish.aundhe@intel.com></a>
Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
ReserveSpace.cpp changes are mostly eliminated/no collector specific
</pre>
</blockquote>
<pre wrap="">code.
</pre>
<blockquote type="cite">
<pre wrap="">Hi Kishor,
On 9/4/18 10:41 PM, Kharbas, Kishor wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi,
I have uploaded implementation for parallel scavenge at
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekkharbas/8204908/webrev-8204908.00">http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.00</a>
Majority of the implementation is handled in two new files -
</pre>
</blockquote>
<pre wrap="">adjoiningGenerationsForHeteroHeap.cpp and
</pre>
</blockquote>
<pre wrap="">psFileBackedVirtualspace.cpp.
</pre>
<blockquote type="cite">
<pre wrap="">Would love to hear your feedback and suggestions.
Sorry for late review.
----------------
General comments.
1. Looks like this patch is not based on latest repository, as it
fails with - Wreorder issue.
2. I see many wrong alignment location of having only 2 spaces after
new line that is continued.
e.g.
a->b(c,
__d, xxxx); // 2 spaces
this should be
a->b(c,
_____d, xxx); // 'd' should locate under 'c' as those are in the same
</pre>
</blockquote>
</blockquote>
<pre wrap="">context.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">3. I see assertion failures with below options combinations. There
could be more... Please run jtreg tests before posting webrev.
-XX:+UseLargePages -XX:+UseSHM -version -XX:+UseLargePages
-XX:+UseNUMA -version -XX:+UseLargePages - XX:AllocateHeapAt=.
-version
=========================
src/hotspot/share/gc/parallel/adjoiningGenerations.cpp
- Copyright update
43 _virtual_spaces(new AdjoiningVirtualSpaces(old_young_rs,
policy->min_old_size(),
44 policy->min_young_size(), alignment) ) {
- Wrong alignment?
=========================
src/hotspot/share/gc/parallel/adjoiningGenerations.hpp
- Copyright update
62 AdjoiningGenerations();
- Why we need this ctor?
=========================
/src/hotspot/share/gc/parallel/adjoiningVirtualSpaces.hpp
- Copyright update
88 virtual PSVirtualSpace* high() { return _high; }
89 virtual PSVirtualSpace* low() { return _low; }
- Those methods don't need 'virtual' specifier as high()/low()
methods are only used at ctor of AdjoiningGenerations. The other
calling site is ctor of AdjoiningGenerationsForHeteroHeap but it is
used another type of
high()/low() which returns _young_vs or _old_vs.
=========================
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp
- no comments.
=========================
src/hotspot/share/gc/parallel/psOldGen.cpp
=========================
src/hotspot/share/gc/parallel/psOldGen.hpp
32 #include "gc/parallel/psFileBackedVirtualspace.hpp"
- Include this header file at cpp seems better rather than hpp.
=========================
src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.cpp
1 /*
2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
3 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE
</pre>
</blockquote>
<pre wrap="">HEADER.
</pre>
<blockquote type="cite">
<pre wrap="">- Wrong alignment. The second/below star should locate under first
line
</pre>
</blockquote>
<pre wrap="">star.
</pre>
<blockquote type="cite">
<pre wrap="">- Similarily there are more wrong alignment locations.
. Line 49, 50
. 52, 53
. 54, 55
. 64, 65
. 67, 68 69 70
. 72, 73 74 75 76
. 79, 80
. 81, 82
. 85, 86
. 87, 88
. 106, 107 108 109
. 114, 115
. 166, 167 168
. 178, 179 180
. 186, 187 188
. 218, 219 220
. 230, 231 232
. 239, 240 241
59 // Initialize the virtual spaces. Then pass the
60 // a virtual to each generation for initialization of the
- Then pass 'the a' virtual to each generation. 'the a'?
64 (static_cast
<HeteroVirtualSpaces*>(_virtual_spaces))->initialize(max_old_byte_si
ze
,
init_old_byte_size,
65 init_young_byte_size);
- Just making 'initilize' method as 'virtual' seems better.
39
</pre>
</blockquote>
</blockquote>
<pre wrap="">AdjoiningGenerationsForHeteroHeap::AdjoiningGenerationsForHeteroHeap(
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">ReservedSpace
old_young_rs, size_t total_size_limit,
40 GenerationSizer* policy, size_t alignment) :
_total_size_limit(total_size_limit) {
- Wrong alirnment at line 40. 'Generation' should be under
</pre>
</blockquote>
</blockquote>
<pre wrap="">'ReservedSpace'
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">at line 39.
49 _virtual_spaces = new HeteroVirtualSpaces(old_young_rs,
min_old_byte_size, 70 (static_cast
<HeteroVirtualSpaces*>(_virtual_spaces))->max_young_size());
75 (static_cast
<HeteroVirtualSpaces*>(_virtual_spaces))->max_old_size(),
- Instead assigning at line 49 and then cast to
HeteroVirtualSpaces*, create/initialize HeteroVirtualSpaces, get
max_young/old_size() and the assign to _virtual_spaces would be better
</pre>
</blockquote>
</blockquote>
<pre wrap="">alternative.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap=""> 109 _min_young_byte_size(min_yg_byte_size),
_max_total_size(max_total_size) {
110 _max_old_byte_size = _max_total_size - _min_young_byte_size;
111 _max_young_byte_size = _max_total_size - _min_old_byte_size;
- _max_old_byte_size and _max_young_byte_size can move to
initialization list.
117 // This the reserved space exclusively for old generation.
122 // This the reserved space exclusively for young generation.
- Missing 'is'? i.e. // This *is* the reserved space exclusively for
old generation.
131 vm_exit_during_initialization("Could not reserve enough space
</pre>
</blockquote>
</blockquote>
<pre wrap="">for "
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap=""> 132 "object heap");
- 'object heap' can stay same line. Or changing align is necessary.
137 vm_exit_during_initialization("Could not reserve enough space
</pre>
</blockquote>
</blockquote>
<pre wrap="">for "
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap=""> 138 "object heap");
- 'object heap' can stay same line. Or changing align is necessary.
152 if (uncommitted_in_old > 0) {
- This condition seems redundant as uncommitted_in_old is type of
</pre>
</blockquote>
</blockquote>
<pre wrap="">size_t.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap=""> 159 if (bytes_needed == 0) {
160 return true;
161 }
- This condition, can move to inside of 'if (uncommitted_in_old > 0)'
condition because if uncommitted_in_old is zero, there's no way
bytes_needed to be zero - bytes_needed is guaranteed not to be zero
from caller site, so safe to move between line 154 and 155. The
condition to return true is 'uncommitted_size == bytes_needed &&
success of expand_by()'.
176 bool ret = _young_vs->shrink_by(shrink_size);
177 assert(ret, "We should be able to shrink young space");
- I would prefer to add more conditions below if we fails to shrink.
i.e. just assert seems not enough.
189 _old_vs->expand_by(bytes_to_add_in_old);
- Check the return value of expand_by().
211 if (bytes_needed == 0) {
212 return true;
213 }
- Same as 'adjust_boundary_down()'
244 DEBUG_ONLY(size_t total_size_after =
_young_vs->reserved_size()
+ _old_vs->reserved_size());
245 assert(total_size_after == total_size_before, "should be
equal");
- Why adjust_boundary_up() doesn't have this kind of check?
=========================
src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.hpp
1 /*
2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
3 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE
</pre>
</blockquote>
<pre wrap="">HEADER.
</pre>
<blockquote type="cite">
<pre wrap=""> 4 *
- Wrong alignment. The second/below star should locate under first
line
</pre>
</blockquote>
<pre wrap="">star.
</pre>
<blockquote type="cite">
<pre wrap=""> 37 size_t total_size_limit() {
38 return _total_size_limit;
39 }
- I don't think we need this but if you prefer to have, add 'const'.
45 PSVirtualSpace* _young_vs;
46 PSVirtualSpace* _old_vs;
- Can't we use 'AdjoiningVirtualSpaces::_low' and '_high' instead?
If it is not the case, adding high(),low() may result in confusion
between
AdjoiningVirtualSpaces::high() and low(). So use other name for
current HeteroVirtualSpaces::high()/low().
55 HeteroVirtualSpaces(ReservedSpace rs,
56 size_t min_old_byte_size,
57 size_t min_young_byte_size, size_t max_total_size,
58 size_t alignment);
- Wrong alignment. Line 56 ~ 58 should be same as the location of
'ReservedSpace' at line 55.
67 size_t max_young_size() { return _max_young_byte_size; }
68 size_t max_old_size() { return _max_old_byte_size; }
- 'const'?
70 void initialize(size_t initial_old_reserved_size, size_t
init_low_byte_size,
71 size_t init_high_byte_size);
- Wrong alignment
82 size_t reserved_byte_size();
- 'const'?
=========================
src/hotspot/share/gc/parallel/psFileBackedVirtualspace.cpp
1 /*
2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
- Wrong alignment. The second/below star should locate under first
line
</pre>
</blockquote>
<pre wrap="">star.
</pre>
<blockquote type="cite">
<pre wrap=""> 36 if (_fd == -1) {
37 return;
38 }
- I prefer to have 'initialize()' method to handle when fails to
create the heap file. Current implementation seems easy to call
additional
</pre>
</blockquote>
<pre wrap="">'initialize()'.
</pre>
<blockquote type="cite">
<pre wrap=""> Ctor of PSVirtualSpace doesn't have any failure path(e.g.
allocation), so no further check is needed around line 18:psOldGen.cpp.
But yours is different, so something should be checked.
33 _mapping_succeeded = false;
34 _file_path = path;
46 _mapping_succeeded = true;
47 _special = true;
- Can move to initialization list with proper initial value.
55 assert(special(), "_special should always be true for
PSFileBackedVirtualSpace");
66 assert(special(), "_special should always be true for
PSFileBackedVirtualSpace");
- For these 2, can we have more specific message? e.g. to include
meaning of expand or shrink.
=========================
src/hotspot/share/gc/parallel/psFileBackedVirtualspace.hpp
1 /*
2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
- Wrong alignment. The second/below star should locate under first
line
</pre>
</blockquote>
<pre wrap="">star.
</pre>
<blockquote type="cite">
<pre wrap=""> 37 PSFileBackedVirtualSpace(ReservedSpace rs, const char*
file_path);
38 bool expand_by(size_t bytes);
- Just recommendation to add new line between 37 and 38.
43 #endif //
</pre>
</blockquote>
<pre wrap="">SHARE_VM_GC_PARALLEL_PSFILEBACKEDVIRTUALSPACE_HPP
</pre>
<blockquote type="cite">
<pre wrap="">- Last line is not terminated with a newline.
Thanks,
Sangheon
</pre>
<blockquote type="cite">
<pre wrap="">I will post G1 GC implementation in a few days.
Thanks
Kishor.
</pre>
<blockquote type="cite">
<pre wrap="">-----Original Message-----
From: <a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a>
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">[<a class="moz-txt-link-freetext" href="mailto:sangheon.kim@oracle.com">mailto:sangheon.kim@oracle.com</a>]
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Sent: Thursday, August 30, 2018 4:06 PM
To: Kharbas, Kishor <a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>; Thomas Stüfe
<a class="moz-txt-link-rfc2396E" href="mailto:thomas.stuefe@gmail.com"><thomas.stuefe@gmail.com></a>; Thomas Schatzl
</pre>
</blockquote>
</blockquote>
<pre wrap=""><a class="moz-txt-link-rfc2396E" href="mailto:thomas.schatzl@oracle.com"><thomas.schatzl@oracle.com></a>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Cc: <a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>; Hotspot dev runtime
<hotspot- <a class="moz-txt-link-abbreviated" href="mailto:runtime-dev@openjdk.java.net">runtime-dev@openjdk.java.net</a>>; Viswanathan, Sandhya
<a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>; Aundhe, Shirish
<a class="moz-txt-link-rfc2396E" href="mailto:shirish.aundhe@intel.com"><shirish.aundhe@intel.com></a>
Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
ReserveSpace.cpp changes are mostly eliminated/no collector
specific
</pre>
</blockquote>
</blockquote>
<pre wrap="">code.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Hi Kishor,
On 8/30/18 11:55 AM, Kharbas, Kishor wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Sangheon,
So far I don’t see a need to do so. I will post my
implementation code
</pre>
</blockquote>
<pre wrap="">today or tomorrow, if we see a need or any simplification by
changing classes in VirtualSpace.hpp, we can discuss that.
Okay.
Thanks,
Sangheon
</pre>
<blockquote type="cite">
<pre wrap="">Regards,
-Kishor
</pre>
<blockquote type="cite">
<pre wrap="">-----Original Message-----
From: <a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a>
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">[<a class="moz-txt-link-freetext" href="mailto:sangheon.kim@oracle.com">mailto:sangheon.kim@oracle.com</a>]
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Sent: Wednesday, August 29, 2018 10:17 AM
To: Kharbas, Kishor <a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>; Thomas Stüfe
<a class="moz-txt-link-rfc2396E" href="mailto:thomas.stuefe@gmail.com"><thomas.stuefe@gmail.com></a>; Thomas Schatzl
</pre>
</blockquote>
</blockquote>
<pre wrap=""><a class="moz-txt-link-rfc2396E" href="mailto:thomas.schatzl@oracle.com"><thomas.schatzl@oracle.com></a>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Cc: <a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>; Hotspot dev runtime
<hotspot- <a class="moz-txt-link-abbreviated" href="mailto:runtime-dev@openjdk.java.net">runtime-dev@openjdk.java.net</a>>; Viswanathan, Sandhya
<a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>; Aundhe, Shirish
<a class="moz-txt-link-rfc2396E" href="mailto:shirish.aundhe@intel.com"><shirish.aundhe@intel.com></a>
Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
ReserveSpace.cpp changes are mostly eliminated/no collector
specific
</pre>
</blockquote>
</blockquote>
<pre wrap="">code.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Hi Kishor,
On 8/29/18 9:52 AM, Kharbas, Kishor wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Sangheon,
Thanks for reviewing the design.
Since the collectors do not use them for heap memory, I did
not have to override VirtualSpace
</pre>
</blockquote>
<pre wrap="">Sorry, I meant ReservedSpace and its friends at
share/memory/virtualspace.hpp.
Thanks,
Sangheon
</pre>
<blockquote type="cite">
<pre wrap="">-Kishor
</pre>
<blockquote type="cite">
<pre wrap="">-----Original Message-----
From: <a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a>
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">[<a class="moz-txt-link-freetext" href="mailto:sangheon.kim@oracle.com">mailto:sangheon.kim@oracle.com</a>]
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Sent: Tuesday, August 28, 2018 2:38 PM
To: Kharbas, Kishor <a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>; Thomas Stüfe
<a class="moz-txt-link-rfc2396E" href="mailto:thomas.stuefe@gmail.com"><thomas.stuefe@gmail.com></a>; Thomas Schatzl
</pre>
</blockquote>
</blockquote>
<pre wrap=""><a class="moz-txt-link-rfc2396E" href="mailto:thomas.schatzl@oracle.com"><thomas.schatzl@oracle.com></a>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Cc: <a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>; Hotspot dev runtime
<hotspot- <a class="moz-txt-link-abbreviated" href="mailto:runtime-dev@openjdk.java.net">runtime-dev@openjdk.java.net</a>>; Viswanathan,
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">Sandhya
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap=""><a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>; Aundhe, Shirish
<a class="moz-txt-link-rfc2396E" href="mailto:shirish.aundhe@intel.com"><shirish.aundhe@intel.com></a>
Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
ReserveSpace.cpp changes are mostly eliminated/no collector
specific
</pre>
</blockquote>
</blockquote>
<pre wrap="">code.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Hi Kishor,
On 8/21/18 10:57 AM, Kharbas, Kishor wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi All,
Thank you for your valuable comments and feedback in this
thread so
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">far.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">After taken in all the comments and reading thoroughly
through the code, I
</pre>
</blockquote>
<pre wrap="">feel that the complexity added to virtualSpace.cpp is due to
lack of abstraction in VirtualSpace and at GC level. NV-DIMM
size and file paths are being passed all the way to OS calls.
</pre>
<blockquote type="cite">
<pre wrap="">So I went back to the drawing board and created a high level
design to remove all the pain points of current implementation.
I have uploaded the design at
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap=""><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekkharbas/8202286/Design%20for%20JEP%20JDK">http://cr.openjdk.java.net/~kkharbas/8202286/Design%20for%20JEP%20JDK</a>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">-
</pre>
<blockquote type="cite">
<pre wrap="">8202286.pdf I would love to hear your feedback and
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">suggestions.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Once we get confidence in the design, I will work on the
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">implementation.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">The design looks good to me.
If you are planning to change VirtualSpace at
share/memory/virtualspace.hpp, including it on the design
document would be helpful too.
Probably folks involved in the previous discussions would say
whether the design well covers their concerns or not.
Thanks,
Sangheon
</pre>
<blockquote type="cite">
<pre wrap="">PS:
1. Vinay has transitioned to another team in Intel, so he
won't be able to
</pre>
</blockquote>
<pre wrap="">continue on this jep.
</pre>
<blockquote type="cite">
<pre wrap="">2. If you feel this should be part of JEP discussion thread,
we can take it
</pre>
</blockquote>
<pre wrap="">there.
</pre>
<blockquote type="cite">
<pre wrap="">Thanks and regards,
Kishor
</pre>
<blockquote type="cite">
<pre wrap="">-----Original Message-----
From: Thomas Stüfe [<a class="moz-txt-link-freetext" href="mailto:thomas.stuefe@gmail.com">mailto:thomas.stuefe@gmail.com</a>]
Sent: Friday, June 22, 2018 9:25 AM
To: Thomas Schatzl <a class="moz-txt-link-rfc2396E" href="mailto:thomas.schatzl@oracle.com"><thomas.schatzl@oracle.com></a>
Cc: Awasthi, Vinay K <a class="moz-txt-link-rfc2396E" href="mailto:vinay.k.awasthi@intel.com"><vinay.k.awasthi@intel.com></a>; Paul Su
<a class="moz-txt-link-rfc2396E" href="mailto:paul.su@oracle.com"><paul.su@oracle.com></a>; <a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>;
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">Hotspot
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">dev
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">runtime <a class="moz-txt-link-rfc2396E" href="mailto:hotspot-runtime-dev@openjdk.java.net"><hotspot-runtime-dev@openjdk.java.net></a>;
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">Viswanathan,
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Sandhya
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap=""><a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>; Aundhe, Shirish
<a class="moz-txt-link-rfc2396E" href="mailto:shirish.aundhe@intel.com"><shirish.aundhe@intel.com></a>; Kharbas, Kishor
<a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>
Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
ReserveSpace.cpp changes are mostly eliminated/no collector
specific
</pre>
</blockquote>
</blockquote>
<pre wrap="">code.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Hi Thomas,
On Fri, Jun 22, 2018 at 4:44 PM, Thomas Schatzl
<a class="moz-txt-link-rfc2396E" href="mailto:thomas.schatzl@oracle.com"><thomas.schatzl@oracle.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Thomas,
On Tue, 2018-06-19 at 13:40 +0200, Thomas Stüfe wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Vinay,
On Mon, Jun 18, 2018 at 6:47 PM, Awasthi, Vinay K
<a class="moz-txt-link-rfc2396E" href="mailto:vinay.k.awasthi@intel.com"><vinay.k.awasthi@intel.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Thomas,
Os::commit_memory calls map_memory_to_file which is
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">same
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">as
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">os::reserve_memory.
I am failing to see why os::reserve_memory can call
map_memory_to_file (i.e. tie it to mmap) but
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">commit_memory
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">can't...
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Before this patch, commit_memory never dealt with
incrementally committing pages to device so there has to
be a way to pass file descriptor and offset. Windows has
no such capability to manage incremental commits. All
other OSes do and that is why map_memory_to_file is used
(which
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">by
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">the way also works on Windows).
</pre>
</blockquote>
<pre wrap="">AIX uses System V shared memory by default, which follows
a different allocation scheme (semantics more like
Windows
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">VirtualAlloc...
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">calls).
But my doubts are not limited to that one, see my earlier
replies and those of others. It really makes sense to
step back one step and discuss the JEP first.
</pre>
</blockquote>
<pre wrap="">There is already a discussion thread as David mentioned
(<a class="moz-txt-link-freetext" href="http://mail.op">http://mail.op</a>
enjdk.java.net/pipermail/hotspot-gc-dev/2018-
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">May/022092.html)
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">that
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">so far nobody answered to.
</pre>
</blockquote>
<pre wrap="">Ah, I thought he wanted to have the JEP discussed in the
comments section of the JEP itself.
</pre>
<blockquote type="cite">
<pre wrap="">I believe discussion about contents the JEP and the
implementation should be separate.
</pre>
</blockquote>
<pre wrap="">makes sense.
</pre>
<blockquote type="cite">
<pre wrap="">So far what I gathered from the responses to the
implementation, the proposed idea itself is not the issue
here (allowing the use of NVDIMM memory for parts of the
heap for allowing the use of larger heaps to improve
overall performance; I am not saying that the text doesn't
need a bit more work :) ), but rather how an
implementation of this JEP
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">should proceed.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">I have no problem with adding NVDIMM support. I think it is
a cool
</pre>
</blockquote>
</blockquote>
<pre wrap="">feature.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Also, the awkwardness off the memory management
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">abstraction
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">layer
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">in hotspot has always been a sore point with me (I
originally implemented the AIX mm layer in os_aix.cpp, and
those are painful memories). So, I have a lot of sympathy
for Vinays
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">struggles.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Unfortunately not much time atm, but I will respond to your
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">mail.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Let's discuss the non-implementation stuff in that thread.
Vinay or I can repost the proposal email if you do not
have it any more so that answers will be in-thread.
</pre>
</blockquote>
<pre wrap="">Okay, sounds good.
Thanks,
Thomas
(one of us should change his name to make this less
confusing
:-)
</pre>
<blockquote type="cite">
<pre wrap="">Thanks,
Thomas
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<br>
</body>
</html>