<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Kishor,<br>
<br>
Forgot to replying on your previous question of:<br>
<blockquote type="cite">
<pre wrap=""><blockquote type="cite"><pre wrap="">2. New tests related general comments:
- As AllocateOldGenAt feature will not be tested regularly, new tests should
be excluded by default.
</pre></blockquote></pre>
<pre wrap="">[Kharbas, Kishor] How do I do this? Should I remove these test from TEST.groups?
</pre>
</blockquote>
Yes, exclude from all gc test groups.<br>
<br>
For exmpale:<br>
tier1_gc_1 = \<br>
gc/epsilon/ \<br>
gc/g1/ \<br>
-gc/g1/ihop/TestIHOPErgo.java<br>
<b>-gc/{nvdimm}</b><br>
<br>
tier1_gc_2 = \<br>
gc/ \<br>
-gc/epsilon/ \<br>
<b>-gc/{nvdimm}</b><br>
<br>
hotspot_not_fast_gc = \<br>
:hotspot_gc \<br>
-:tier1_gc<br>
<b>-gc/{nvdimm}</b><br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<div class="moz-cite-prefix">On 12/5/18 3:02 PM,
<a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:3ffe8790-1cc0-7811-8010-d1c8088155ff@oracle.com">Hi
Kishor,
<br>
<br>
On 12/5/18 12:51 AM, Kharbas, Kishor wrote:
<br>
<blockquote type="cite">Hi Stefan, Sangheon,
<br>
I have another webrev update -
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.06">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.06</a>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.05_to_06">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.05_to_06</a>
<br>
<br>
Changes are:
<br>
1. While testing the new evacuation handling mechanism, I
identified some issues and fixed them. The implementation has
been tested by tracing the code path through the debugger.
<br>
Based on the fix, here is an explanation for evacuation
failure handling -
<br>
1) The implementation is based on the axiom that
for every region in dram where is a unavailable (not
'committed') region in nv-dimm. By borrowing such regions, we
can avoid evacuation failure for young objects and thus avoid
young->old region conversion.
<br>
2) We cannot use borrowed regions for old objects.
Since in worst case for evacuating 'n' young and 'm' old regions
we need 'n + m' destination regions. And we may need to borrow
'n+m' from nv-dimm. But we can only guarantee 'n' borrowed
regions.
<br>
So we use borrowed region only for copying
young objects, else we cannot avoid young->old conversion and
will need a special region type as in current implementation
anyway.
<br>
3) When we cannot allocate a new region for
OldGCAllocRegion, we borrow a region from hrm() to use as
allocation region. We release (un-commit) that many number of
regions in adjust_dram_regions() which is called at end of GC.
<br>
4) So we may be in situation where there is valid
old plab and OldGCAllocRegion but we can only use them when
source is young. So we add another clause to the if() statement
at the beginning of copy_to_survivor_space() to check for this
condition.
<br>
For ease of review I extracted evacuation failure handling
changes here -
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev_EvacuationFailure">http://cr.openjdk.java.net/~kkharbas/8211425/webrev_EvacuationFailure</a>
<br>
</blockquote>
I have some comments.
<br>
<br>
1. Have you test enough for evacuation failure handling case? i.e.
it would be better to have tests for these cases.
<br>
e.g. How can we guarantee the total heap will be less than Xmx
after GC if we followed 'borrowing region path'.
<br>
Couldn't shrinking dram fail after borrowing a region?
(heterogeneousHeapRegionManager.cpp:65)
<br>
<br>
2. Do we still need pre-mature old type after introducing
'borrowing concept'? (Stefan also mentioned about this)
<br>
<br>
3. (Again) Please don't merge ParallelGC patch(8211424) into G1
Patch(8211425) as those should be committed separately. And this
email thread is to review G1. :) i.e. you should commit identical
reviewed patch, not modifying it (to split Parallel part).
<br>
<br>
--------------------------------------
<br>
src/hotspot/share/gc/g1/g1CollectedHeap.cpp
<br>
4561 new_alloc_region =
static_cast<HeterogeneousHeapRegionManager*>(hrm())->borrow_old_region_for_gc();<br>
- This can be moved to hetero hrm. Stefan also mentioned this.
<br>
<br>
<br>
--------------------------------------
<br>
src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
<br>
227 // Third clause: if it is a heterogenous heap, we borrow
regions from nv-dimm for old gc allocation.
<br>
228 // This region can only be used to evacate young objects,
so we check the state of the object being copied.
<br>
- Something like this:
<br>
// Third clause: if it is a heterogenous heap, we can borrow
regions from nv-dimm for old region allocation. And these regions
should be only used to evacuate young objects.
<br>
<br>
229 if (_old_gen_is_full && dest_state.is_old() ||
<br>
230 (_g1h->is_hetero_heap() && state.is_old()
&&
static_cast<HeterogeneousHeapRegionManager*>(_g1h->hrm())->is_old_full())
) {
<br>
- It would be better to have another parenthesis at line 229.
<br>
<br>
<br>
--------------------------------------
<br>
src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.cpp
<br>
65 uint num = shrink_dram(_no_borrowed_regions);
<br>
66 assert(num == _no_borrowed_regions, "Should be able to
uncommit the number of regions borrowed for evacuation failure
handling");
<br>
- Couldn't we fail at line 65?
<br>
<br>
<br>
--------------------------------------
<br>
src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.hpp
<br>
45 bool _is_old_full;
<br>
- This name looks mis-leading. It sets true when we temporarily
borrow a region from nvdimm during GC. To align with
'_no_borrowed_regions', something like 'has_borrowed_regions'?
<br>
<br>
135 bool is_old_full();
<br>
- 'const'?
<br>
<br>
<br>
<blockquote type="cite">2. There is a fix for incorrect handling
of humongous allocations under certain situations.
<br>
3. My jtreg tests are running, will provide the results when
they finish.
<br>
</blockquote>
I guess you are running serviceability as well?
<br>
<br>
Thanks,
<br>
Sangheon
<br>
<br>
<br>
<blockquote type="cite">
<br>
Let me know your feedback.
<br>
<br>
Thank you.
<br>
Kishor
<br>
<br>
<blockquote type="cite">-----Original Message-----
<br>
From: Kharbas, Kishor
<br>
Sent: Monday, December 3, 2018 11:48 PM
<br>
To: <a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a>; Stefan Johansson
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:stefan.johansson@oracle.com"><stefan.johansson@oracle.com></a>
<br>
Cc: '<a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>'
<a class="moz-txt-link-rfc2396E" href="mailto:hotspot-gc-dev@openjdk.java.net"><hotspot-gc-dev@openjdk.java.net></a>;
<br>
'Hotspot dev runtime'
<a class="moz-txt-link-rfc2396E" href="mailto:hotspot-runtime-dev@openjdk.java.net"><hotspot-runtime-dev@openjdk.java.net></a>;
<br>
Viswanathan, Sandhya <a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>;
Kharbas, Kishor
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>
<br>
Subject: RE: RFR(M): 8211425: Allocation of old generation of
java heap on
<br>
alternate memory devices - G1 GC
<br>
<br>
Hi Sangheon and Stefan,
<br>
<br>
I have updated webrev for this feature,
<br>
<br>
1. Webrev with Stefan's comments on Parallel GC (abstraction
related
<br>
comments) :
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.04">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.04</a>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.03_to_04">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.03_to_04</a>
<br>
<br>
2. Webrev with evacuation failure handling :
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.05">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.05</a>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.04_to_05">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.04_to_05</a>
<br>
Testing underway.
<br>
<br>
3. Explanation of evacuation failure handling:
<br>
1) The implementation is based on the axiom that
for every region in
<br>
dram where is a unavailable (not 'committed') region in
nv-dimm. By
<br>
borrowing such regions, we can avoid evacuation failure for
young objects
<br>
and thus avoid young->old region conversion.
<br>
2) We cannot use borrowed regions for old
objects. Since in worst
<br>
case for evacuating 'n' young and 'm' old regions we need 'n +
m' destination
<br>
regions. And we may need to borrow 'n+m' from nv-dimm. But we
can only
<br>
guarantee 'n' borrowed regions.
<br>
So we use borrowed region only for copying
young objects, else we
<br>
cannot avoid young->old conversion and will need a special
region type as in
<br>
current implementation anyway.
<br>
3) When we cannot allocate a new region for
OldGCAllocRegion, we
<br>
borrow a region from hrm() to use as allocation region. We
sort of release
<br>
that many number of regions in adjust_dram_regions() which is
called at end
<br>
of GC.
<br>
4) When G1ParScanThreadState: _old_gen_is_full is
true, we only let
<br>
young objects be allocated from the borrowed region. There
might be a race
<br>
condition here, when one thread enters plab_allocate() while
other thread
<br>
determines and set _old_gen_is_full.
<br>
But we can tolerate this race condition.
<br>
<br>
4. I am testing all tier1 tests and appCDS tests with the
combined patch. Will
<br>
provide an update soon.
<br>
<br>
Thanks,
<br>
Kishor
<br>
<br>
<blockquote type="cite">-----Original Message-----
<br>
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>]
<br>
Sent: Sunday, December 2, 2018 2:49 PM
<br>
To: Kharbas, Kishor <a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>; Stefan
Johansson
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:stefan.johansson@oracle.com"><stefan.johansson@oracle.com></a>
<br>
Cc: '<a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>'
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:hotspot-gc-dev@openjdk.java.net"><hotspot-gc-dev@openjdk.java.net></a>;
<br>
'Hotspot dev runtime'
<a class="moz-txt-link-rfc2396E" href="mailto:hotspot-runtime-dev@openjdk.java.net"><hotspot-runtime-dev@openjdk.java.net></a>;
<br>
Viswanathan, Sandhya <a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>
<br>
Subject: Re: RFR(M): 8211425: Allocation of old generation
of java
<br>
heap on alternate memory devices - G1 GC
<br>
<br>
Hi Kishor,
<br>
<br>
On 11/29/18 12:19 PM, Kharbas, Kishor wrote:
<br>
<blockquote type="cite">Hi Sangheon,
<br>
<br>
Thank you for reviewing the webrev.
<br>
1. Based on your feedback I have newer webrev. There are
some
<br>
</blockquote>
comments inline about AppCDS (top of previous email).
<br>
<blockquote type="cite">2. Webrev with your feedback on G1
GC patch :
<br>
</blockquote>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.02/">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.02/</a>
<br>
<blockquote type="cite"><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.01_to_02/">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.01_to_02/</a>
<br>
</blockquote>
Webrev.02 looks good.
<br>
<br>
<blockquote type="cite">3. I merged the Parallel GC patch
with your latest comment,
<br>
consolidated jtreg tests and combined webrev is :
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.03">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.03</a>
<br>
</blockquote>
I meant to work based on Parallel GC patch so that your G1
patch
<br>
doesn't need to comment out Paralell GC test case at newly
added tests.
<br>
<br>
I only checked commented out parts of Parallel GC are
enabled. This
<br>
looks good too.
<br>
I would expect that you completed running whole your
patches(G1 and
<br>
Parallel GC) together without any failures.
<br>
<br>
<blockquote type="cite">4. I moved the CSR to finalized.
Thanks for your review. We will
<br>
wait to
<br>
</blockquote>
hear from CSR committee.
<br>
Okay, I saw it is approved now.
<br>
<br>
<blockquote type="cite">5. I am working on Stefan's latest
feedback on handling of
<br>
evacuation
<br>
</blockquote>
failure.
<br>
OK.
<br>
<br>
Thanks,
<br>
Sangheon
<br>
<br>
<br>
<blockquote type="cite">Regards,
<br>
Kishor
<br>
<br>
<blockquote type="cite">-----Original Message-----
<br>
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>]
<br>
Sent: Tuesday, November 27, 2018 9:25 PM
<br>
To: Kharbas, Kishor <a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>;
Stefan Johansson
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:stefan.johansson@oracle.com"><stefan.johansson@oracle.com></a>
<br>
Cc: '<a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>' <hotspot-gc-
<br>
</blockquote>
</blockquote>
<a class="moz-txt-link-abbreviated" href="mailto:dev@openjdk.java.net">dev@openjdk.java.net</a>>;
<br>
<blockquote type="cite">
<blockquote type="cite">'Hotspot dev runtime'
<a class="moz-txt-link-rfc2396E" href="mailto:hotspot-runtime-dev@openjdk.java.net"><hotspot-runtime-dev@openjdk.java.net></a>;
<br>
Viswanathan, Sandhya
<a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>
<br>
Subject: Re: RFR(M): 8211425: Allocation of old
generation of java
<br>
heap
<br>
</blockquote>
</blockquote>
on
<br>
<blockquote type="cite">
<blockquote type="cite">alternate memory devices - G1 GC
<br>
<br>
Hi Kishor,
<br>
<br>
On 11/19/18 6:20 PM, Kharbas, Kishor wrote:
<br>
<blockquote type="cite">Hi Sangheon,
<br>
<br>
Here is the incremental webrev -
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00_to_01">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00_to_01</a>
<br>
</blockquote>
Thanks for the incremental webrev.
<br>
Here are some comments for webrev.01.
<br>
<br>
1. General comments:
<br>
- Any update with AppCDS tests? (You mentioned this on
your
<br>
previous
<br>
</blockquote>
</blockquote>
email)
<br>
<blockquote type="cite"> [Kharbas, Kishor] I completed
testing of AppCDS with : 1) original
<br>
code. 2)
<br>
</blockquote>
patch applied, but flag not used. 3) patch applied, flag
set.
<br>
<blockquote type="cite">
Test results at :
<br>
</blockquote>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/JT_AppCDS">http://cr.openjdk.java.net/~kkharbas/8211425/JT_AppCDS</a>
<br>
<blockquote type="cite">
1) and 2) both show "Test results:
<br>
passed: 120; failed: 1;
<br>
</blockquote>
error: 2"
<br>
<blockquote type="cite">
3) No tests are run, all are
<br>
skipped. Do you know if
<br>
</blockquote>
appCDS does not work with certain flags. I use jtreg flag
<br>
-javaoptions:"- XX:+UnlockExperimentalVMOptions -
<br>
</blockquote>
XX:AllocateOldGenAt=/home/kishor".
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">- Please test with ParallelGC
patch added. i.e. Applied both
<br>
ParallelGC and
<br>
</blockquote>
</blockquote>
G1
<br>
<blockquote type="cite">
<blockquote type="cite">parts, so that you don't need to
comment out ParallelGC on new tests.
<br>
<br>
</blockquote>
[Kharbas, Kishor] The tests pass with merged patch,
testing both
<br>
ParallelGC
<br>
</blockquote>
and G1.
<br>
<blockquote type="cite">
<blockquote type="cite">2. New tests related general
comments:
<br>
- As AllocateOldGenAt feature will not be tested
regularly, new
<br>
tests
<br>
</blockquote>
</blockquote>
should
<br>
<blockquote type="cite">
<blockquote type="cite">be excluded by default.
<br>
</blockquote>
[Kharbas, Kishor] How do I do this? Should I remove these
test from
<br>
</blockquote>
TEST.groups?
<br>
<blockquote type="cite">
<blockquote type="cite">- New tests need filters for other
unsupported GCs.
<br>
e.g. @requires vm.gc=="null" | vm.gc.G1 |
vm.gc.Parallel
<br>
</blockquote>
[Kharbas, Kishor] Done.
<br>
<blockquote type="cite">- Are there any reason for
printing stdout in new tests? i.e. looks
<br>
like we don't need
'System.out.println(output.getStdout());'
<br>
<br>
</blockquote>
[Kharbas, Kishor] not really, I checked that output is
dumped to
<br>
stdout of
<br>
</blockquote>
jtreg anyway. So I removed this.
<br>
<blockquote type="cite">
<blockquote type="cite">----------------------
<br>
src/hotspot/share/gc/g1/g1CollectedHeapHeap.hpp
<br>
45 #include "gc/g1/heapRegionManager.hpp"
<br>
46 #include
"gc/g1/heterogeneousHeapRegionManager.hpp"
<br>
47 #include "gc/g1/heapRegionSet.hpp"
<br>
- Line 46 should be below line 47.
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/gc/g1/g1CollectorPolicy.hpp
<br>
41 virtual size_t heap_reservation_size_bytes();
<br>
- const?
<br>
<br>
45 size_t _heap_reservation_size_bytes;
<br>
- Do we really need this variable? Just returning
<br>
2*_max_heap_byte_size seems enough. In this case, adding
virtual
<br>
size_t
<br>
heap_reservation_size_bytes() seems enough.
<br>
- Regarding naming, how about
heap_reserved_size_bytes()?
<br>
<br>
51 size_t heap_reservation_size_bytes();
<br>
- I would prefer to have 'virtual' for readability. I
saw you
<br>
reflected this
<br>
</blockquote>
</blockquote>
kind
<br>
<blockquote type="cite">
<blockquote type="cite">of my comment in other places so
asking here too.
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
<br>
25 #include "logging/log.hpp"
<br>
- precompiled.hpp comes first
<br>
<br>
180 return false;
<br>
181 log_error(gc, init)("Could not create file
for Old
<br>
generation at location %s", AllocateOldGenAt);
<br>
- We never reach line 181 because of line 180? :)
<br>
<br>
196
<br>
<br>
</blockquote>
</blockquote>
</blockquote>
G1RegionToHeteroSpaceMapper::G1RegionToHeteroSpaceMapper(Reserve
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">dSpace rs,
<br>
- My previous comment was about splitting initialization
part(which
<br>
does actual file-heap mapping) from ctor, so that we can
avoid
<br>
failure during construction. After construction, call
initialize
<br>
method to do file mapping and then report any failures
if have.
<br>
<br>
196
<br>
<br>
</blockquote>
</blockquote>
</blockquote>
G1RegionToHeteroSpaceMapper::G1RegionToHeteroSpaceMapper(Reserve
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">dSpace rs,
<br>
197 size_t actual_size,
<br>
198 size_t page_size,
<br>
199 size_t alloc_granularity,
<br>
200 size_t commit_factor,
<br>
201 MemoryType type) :
<br>
202 G1RegionToSpaceMapper(rs, actual_size,
page_size,
<br>
alloc_granularity, commit_factor, type),
<br>
203 _num_committed_dram(0),
_num_committed_nvdimm(0),
<br>
_success(false) {
<br>
- Looks like a bad alignment. I think parameters and
initialization
<br>
list(parental class as well) should be distinguishable.
<br>
<br>
<br>
295 delete mapper;
<br>
- Existing issue(no action needed): current mapper code
doesn't
<br>
have a destructor.
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/gc/g1/heapRegionManager.cpp
<br>
29 #include "gc/g1/heapRegionManager.inline.hpp"
<br>
30 #include
"gc/g1/heterogeneousHeapRegionManager.hpp"
<br>
31 #include "gc/g1/heapRegionSet.inline.hpp"
<br>
- Move line 30 after line 31.
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/gc/g1/heapRegionManager.hpp
<br>
<br>
117 FreeRegionList _free_list;
<br>
118 void make_regions_available(uint index, uint
num_regions =
<br>
1,
<br>
WorkGang* pretouch_gang = NULL);
<br>
- New line between 117 and 118 please.
<br>
<br>
130 static HeapRegionManager*
create_manager(G1CollectedHeap*
<br>
heap,
<br>
CollectorPolicy* policy);
<br>
- I see build failure in solaris.
<br>
open/src/hotspot/share/gc/g1/heapRegionManager.hpp",
line 129:
<br>
</blockquote>
</blockquote>
</blockquote>
Error:
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">CollectorPolicy is not defined.
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/gc/g1/heapRegionType.cpp
<br>
- Copyright update
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/gc/shared/gcArguments.cpp
<br>
<br>
68 "AllocateOldGenAt not supported for
selected GC.\n");
<br>
- "AllocateOldGenAt *is* not supported ..."
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/prims/whitebox.cpp
<br>
510 return (jlong)(Universe::heap()->base()
+ start_region
<br>
* HeapRegion::GrainBytes);
<br>
- g1h->base() is same, isn't it? There are more lines
that using
<br>
Universe::heap()->base().
<br>
<br>
524 }
<br>
525 else {
<br>
and
<br>
551 }
<br>
552 else {
<br>
- Make at one line. i.e. '} else {'
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/runtime/arguments.cpp
<br>
1625 if (AllocateOldGenAt != NULL) {
<br>
1626 FLAG_SET_ERGO(bool, UseCompressedOops, false);
<br>
1627 return;
<br>
1628 }
<br>
- Wrong alignment. Needs 2 spaces for line 1625~1628.
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/runtime/globals.hpp
<br>
2606 experimental(ccstr, AllocateOldGenAt, NULL,
<br>
\
<br>
2607 "Directory to use for allocating old
<br>
generation")
<br>
- How about moving AllocateOldGenAt around
AllocateHeapAt option.
<br>
- Change the description similar to AllocateHeapAt as it
explains better.
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.cpp
<br>
29 #include "gc/g1/heapRegionManager.inline.hpp"
<br>
30 #include
"gc/g1/heterogeneousHeapRegionManager.hpp"
<br>
31 #include "gc/g1/heapRegionSet.inline.hpp"
<br>
- Line 30 should be located after line 31.
<br>
<br>
<br>
----------------------
<br>
src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.hpp
<br>
<br>
#ifndef
SHARE_VM_GC_G1_HeterogeneousHeapRegionManager_HPP
<br>
- Should be all uppercase letters.
<br>
<br>
<br>
----------------------
<br>
test/hotspot/jtreg/gc/8202286/TestAllocateOldGenAtMultiple.java
<br>
56 "-Xmx4g
-Xms4g", // 4.
<br>
With larger heap size (UnscaledNarrowOop not possible).
<br>
57 "-Xmx4g -Xms4g
-XX:+UseLargePages", // 5.
<br>
Set UseLargePages.
<br>
58 "-Xmx4g -Xms4g
-XX:+UseNUMA" // 6. Set UseNUMA.
<br>
- It would be better to use smaller heap for test
stability(even
<br>
though this will not be tested regularly) unless you
have any good
<br>
</blockquote>
</blockquote>
</blockquote>
reason for it.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<br>
<blockquote type="cite">I have also filed a CSR at
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK">https://bugs.openjdk.java.net/browse/JDK</a>-
<br>
</blockquote>
8214081
<br>
I reviewed this CSR as well.
<br>
<br>
Thanks,
<br>
Sangheon
<br>
<br>
<br>
<blockquote type="cite">Thank you,
<br>
Kishor
<br>
<br>
<blockquote type="cite">-----Original Message-----
<br>
From: <a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a>
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
[<a class="moz-txt-link-freetext" href="mailto:sangheon.kim@oracle.com">mailto:sangheon.kim@oracle.com</a>]
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">Sent: Sunday, November 18,
2018 11:14 AM
<br>
To: Kharbas, Kishor
<a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>; Stefan Johansson
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:stefan.johansson@oracle.com"><stefan.johansson@oracle.com></a>;
'<a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>'
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:hotspot-gc-dev@openjdk.java.net"><hotspot-gc-dev@openjdk.java.net></a>; 'Hotspot
dev runtime'
<br>
<hotspot- <a class="moz-txt-link-abbreviated" href="mailto:runtime-dev@openjdk.java.net">runtime-dev@openjdk.java.net</a>>
<br>
Cc: Viswanathan, Sandhya
<a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>
<br>
Subject: Re: RFR(M): 8211425: Allocation of old
generation of
<br>
java heap
<br>
</blockquote>
</blockquote>
on
<br>
<blockquote type="cite">
<blockquote type="cite">alternate memory devices - G1
GC
<br>
<br>
Hi Kishor,
<br>
<br>
Could you give us incremental webrev? It will help a
lot for reviewers.
<br>
<br>
Thanks,
<br>
Sangheon
<br>
<br>
<br>
On 11/16/18 6:35 PM, Kharbas, Kishor wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
I have an update to the webrev at :
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.01">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.01</a>
<br>
1. Most of the comments from Sangheon and Stefan
have been
<br>
</blockquote>
</blockquote>
</blockquote>
addressed.
<br>
<blockquote type="cite">
<blockquote type="cite">For ease of reading, I am
copy-pasting below the comments not
<br>
fixed
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
or
<br>
<blockquote type="cite">
<blockquote type="cite">for
<br>
<blockquote type="cite">
<blockquote type="cite">which I have follow up.
<br>
<blockquote type="cite">2. I also ran jtreg tests,
with and without flag using test
<br>
group shown
<br>
</blockquote>
</blockquote>
</blockquote>
below
<br>
<blockquote type="cite">
<blockquote type="cite">(removed tests incompatible
with this flag. Also, for testing, I
<br>
made - XX:+AllocateOldGenAt a no-op instead of error
since many
<br>
tests have
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
sub-
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">tests for all different GCs)
<br>
<blockquote type="cite"> I see same number of
failures (12) for original, patch
<br>
without flag and with flag (no new failures) 3.
Tasks in progress are :
<br>
- AppCDS tests.
<br>
- CSR filing.
<br>
<br>
tier1_old_nvdimm = \
<br>
serviceability/ \
<br>
:tier1_gc \
<br>
:tier1_runtime \
<br>
-gc/serial \
<br>
-gc/parallel \
<br>
-gc/cms \
<br>
-gc/epsilon \
<br>
-gc/TestAllocateHeapAt.java \
<br>
-gc/TestAllocateHeapAtError.java \
<br>
-gc/TestAllocateHeapAtMultiple.java \
<br>
-gc/g1/TestFromCardCacheIndex.java \
<br>
-gc/metaspace/TestMetaspacePerfCounters.java
\
<br>
-gc/metaspace/TestPerfCountersAndMemoryPools.java
\
<br>
-runtime/Metaspace/PrintMetaspaceDcmd.java
<br>
<br>
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
========================================================
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">Comments from Sangheon:
<br>
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
========================================================
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
<br>
<br>
173 void
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
G1RegionToHeteroSpaceMapper::map_nvdimm_space(ReservedSpace
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">rs) {
<br>
- Your previous change of adding AllocateHeapAt
created nv file
<br>
inside
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
of
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">ReservedHeapSpace but
this-AllocateOldGenAt create inside of
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
mapper.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">Couldn't old gen also create
its file near there? I feel like
<br>
creating here
<br>
</blockquote>
</blockquote>
seems
<br>
<blockquote type="cite">
<blockquote type="cite">un-natural.
<br>
<blockquote type="cite">- This is just an idea.
Instead of adding AllocateOldGenAt,
<br>
couldn't re-
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
use
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">AllocateHeapAt and add type
option? e.g. 'all' or 'old' or 'off'.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">If I do this in
ReservedHeapSpace, all of the reserved memory
<br>
will
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
be
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">mapped to nv-dimm. Since here
I want part of reserved memory in
<br>
nv-
<br>
</blockquote>
</blockquote>
dimm,
<br>
<blockquote type="cite">
<blockquote type="cite">I can't use this directly, or
easily modify it for our need.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">I like the idea of
re-using AllocateHeapAt. I however not sure
<br>
how can a '-XX option handle extra option.( I
am thinking of
<br>
something similar to -Xbootclasspath/a(p))
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
========================================================
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.cpp
<br>
34 // expand_by() is called to grow the heap. We
grow into
<br>
nvdimm
<br>
</blockquote>
</blockquote>
</blockquote>
now.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">35 // Dram regions are
committed later as needed during mutator
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
region
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">allocation or
<br>
36 // when young list target length is determined
after gc cycle.
<br>
37 uint
HeapRegionManagerForHeteroHeap::expand_by(uint
<br>
</blockquote>
</blockquote>
</blockquote>
num_regions,
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">WorkGang* pretouch_workers)
{
<br>
38 uint num_expanded =
expand_nvdimm(MIN2(num_regions,
<br>
max_expandable_length() -
total_regions_committed()),
<br>
pretouch_workers);
<br>
39 assert(total_regions_committed() <=
<br>
max_expandable_length(), "must be");
<br>
40 return num_expanded;
<br>
41 }
<br>
- I guess the only reason of not adjusting dram
here is that we
<br>
don't have information of 'is_old'? Looks a bit
weired but I
<br>
don't have any suggestion. (see more below at
<br>
allocate_free_region)
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">Yes, expand_by() is used
to grow the heap at initialization or
<br>
after
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
full
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">GC. Since young generation
target length is decided at a later
<br>
point, I
<br>
</blockquote>
</blockquote>
expand
<br>
<blockquote type="cite">
<blockquote type="cite">in nv-dimm space here.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">Later when young
generation target length is determined,
<br>
</blockquote>
</blockquote>
</blockquote>
adjust_dram_regions() is called to provision the
target number of
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
regions.
<br>
</blockquote>
========================================================
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">Comments from Stefan
<br>
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
========================================================
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">src/hotspot/share/gc/g1/g1CollectedHeap.cpp
<br>
-------------------------------------------
<br>
1642 if (AllocateOldGenAt != NULL) {
<br>
1643 _is_hetero_heap = true;
<br>
1644 max_byte_size *= 2;
<br>
1645 }
<br>
<br>
I would like this to be abstracted out of
G1CollectedHeap, not
<br>
entirely sure how but I'm thinking something like
adding a
<br>
G1HeteroCollectorPolicy which answers correctly on
how much
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
memory
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">needs to be reserved and how
big the heap actually is.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">I abstracted out this in
G1HeteroCollectorPolicy.
<br>
</blockquote>
</blockquote>
1668
G1RegionToSpaceMapper::create_heap_mapper(g1_rs,
<br>
1669
g1_rs.size(),
<br>
1670
page_size,
<br>
1671
HeapRegion::GrainBytes,
<br>
1672 1,
<br>
1673
mtJavaHeap);
<br>
<br>
This function could then also make use of the new
policy,
<br>
something
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
like:
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">create_heap_mapper(g1_rs,
_collector_policy, page_size);
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">Since the mapper needs
to map the reserved memory, Since
<br>
</blockquote>
</blockquote>
</blockquote>
create_heap_mapper() does not need anything from
<br>
collector_policy, I chose to keep the call
unchanged.
<br>
<blockquote type="cite">3925
if(g1h->is_hetero_heap()) {
<br>
3926 if(!r->is_old()) {
<br>
....
<br>
3929 r->set_premature_old();
<br>
3930 }
<br>
3931 } else {
<br>
3932 r->set_old();
<br>
3933 }
<br>
<br>
So if I understand this correctly, premature_old
is used when we
<br>
get evac failures and we use it to force these
regions to be
<br>
included in the next Mixed collection. I'm not
sure this is
<br>
needed, in fact I think one of the cool things
with nv-dimm is
<br>
that we can avoid evac failures. G1 normally needs
to stop
<br>
handing out regions to promote to when there are
no regions
<br>
left, but when using nv-dimm the old
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
regions
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">come from another pool and
we should be able to avoid this case.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">Yes! I had given this a
thought. We can always procure a free
<br>
region
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
in
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">nv-dimm. However if we do
this, during this GC phase there could
<br>
be
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
more
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">regions committed in memory
than Xmx.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">This would mean heap
memory usage increases to more than
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
'Xmx'
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">during some gc phases. Will
this be ok?
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">But again in current
implementation memory usage is more than
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
Xmx
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">anyway, since I allocate
Xmx memory in nv-dimm at start. I do
<br>
this
<br>
</blockquote>
</blockquote>
</blockquote>
because allocating in nv-dimm involves mapping to a
file system,
<br>
which becomes complicated if you expand/shrink file
whenever you
<br>
commit/uncommit regions. But I chose to keep this
only in
<br>
G1regionToSpaceMapper and not design rest of the
code based on
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
this.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">I had to make an
exception for Full GC where I increase the
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
committed
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">regions in nv-dimm before
start of GC, since we need to move all
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
objects
<br>
<blockquote type="cite">
<blockquote type="cite">to
<br>
<blockquote type="cite">
<blockquote type="cite">old generation.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">Let me know what you
think, I like your idea since we don't
<br>
need to
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
add
<br>
<blockquote type="cite">
<blockquote type="cite">more complexity for handling
premature old regions.
<br>
<blockquote type="cite">Thanks and regards,
<br>
Kishor
<br>
<br>
<blockquote type="cite">-----Original Message-----
<br>
From: <a class="moz-txt-link-abbreviated" href="mailto:sangheon.kim@oracle.com">sangheon.kim@oracle.com</a>
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
[<a class="moz-txt-link-freetext" href="mailto:sangheon.kim@oracle.com">mailto:sangheon.kim@oracle.com</a>]
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">Sent: Monday, November 5,
2018 9:39 PM
<br>
To: Kharbas, Kishor
<a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>; Stefan
<br>
Johansson <a class="moz-txt-link-rfc2396E" href="mailto:stefan.johansson@oracle.com"><stefan.johansson@oracle.com></a>;
'hotspot-gc-
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<a class="moz-txt-link-abbreviated" href="mailto:dev@openjdk.java.net">dev@openjdk.java.net</a>'
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite"><a class="moz-txt-link-rfc2396E" href="mailto:hotspot-gc-dev@openjdk.java.net"><hotspot-gc-dev@openjdk.java.net></a>;
'Hotspot dev runtime'
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<hotspot-
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite"><a class="moz-txt-link-abbreviated" href="mailto:runtime-dev@openjdk.java.net">runtime-dev@openjdk.java.net</a>>
<br>
Cc: Viswanathan, Sandhya
<a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>
<br>
Subject: Re: RFR(M): 8211425: Allocation of old
generation of
<br>
java heap on alternate memory devices - G1 GC
<br>
<br>
Hi Kishor,
<br>
<br>
On 11/2/18 2:48 PM, Kharbas, Kishor wrote:
<br>
<blockquote type="cite">Thanks Stefan and
Sangheon for your comments.
<br>
<br>
As suggested, I ran jtreg for serviceability
tests. I see
<br>
failures even without
<br>
</blockquote>
using the new flag.
<br>
<blockquote type="cite">Looks like things are
breaking in mirror classes
<br>
</blockquote>
(jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.*)
because of new
<br>
fields and/or method changes in JVM classes.
<br>
<blockquote type="cite">Is there a bkm for
changing these mirror Java classes when we
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
change
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">JVM
<br>
</blockquote>
classes?
<br>
Sorry I'm not aware of any better known method
than fixing
<br>
one-by-
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
one.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">I am working on
addressing your comments on the patch (had
<br>
been
<br>
</blockquote>
dragged into another project, so I am behind on
this).
<br>
Thanks for your update.
<br>
<br>
Thanks,
<br>
Sangheon
<br>
<br>
<br>
<blockquote type="cite">Thanks
<br>
Kishor
<br>
<br>
<blockquote type="cite">-----Original
Message-----
<br>
From: Stefan Johansson
[<a class="moz-txt-link-freetext" href="mailto:stefan.johansson@oracle.com">mailto:stefan.johansson@oracle.com</a>]
<br>
Sent: Friday, October 26, 2018 7:32 AM
<br>
To: Kharbas, Kishor
<a class="moz-txt-link-rfc2396E" href="mailto:kishor.kharbas@intel.com"><kishor.kharbas@intel.com></a>;
'hotspot-gc-
<br>
<a class="moz-txt-link-abbreviated" href="mailto:dev@openjdk.java.net">dev@openjdk.java.net</a>'
<a class="moz-txt-link-rfc2396E" href="mailto:hotspot-gc-dev@openjdk.java.net"><hotspot-gc-dev@openjdk.java.net></a>;
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
'Hotspot
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">dev
<br>
<blockquote type="cite">
<blockquote type="cite">runtime'
<a class="moz-txt-link-rfc2396E" href="mailto:hotspot-runtime-dev@openjdk.java.net"><hotspot-runtime-dev@openjdk.java.net></a>
<br>
Cc: Viswanathan, Sandhya
<a class="moz-txt-link-rfc2396E" href="mailto:sandhya.viswanathan@intel.com"><sandhya.viswanathan@intel.com></a>
<br>
Subject: Re: RFR(M): 8211425: Allocation of
old generation of
<br>
java heap on alternate memory devices - G1
GC
<br>
<br>
Hi Kishor,
<br>
<br>
I'll start with a few general observations
and then there are
<br>
some specific code comments below.
<br>
<br>
I think the general design looks promising,
not having to set
<br>
a fixed limit for what can end up on NV-dimm
is great. Having
<br>
a separate HeapRegionManager looks like a
good abstraction
<br>
and
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
when
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">adding larger features
like this that is really important.
<br>
<br>
I also agree with Sangheon that you should
do more testing,
<br>
both with and without the featured turned
on. I also
<br>
recommend you to build with pre- compiled
headers disabled,
<br>
to find places where includes have been
forgotten. To
<br>
configure such build add
--disable-precompiled-headers to your
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
configure call.
<br>
<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">On 2018-10-04 04:08,
Kharbas, Kishor wrote:
<br>
<blockquote type="cite">Hi all,
<br>
<br>
Requesting review of the patch for
allocating old generation
<br>
of
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
g1
<br>
<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">gc on alternate
memory devices such nv-dimm.
<br>
<br>
The design of this implementation is
explained on the bug
<br>
page -
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8211425">https://bugs.openjdk.java.net/browse/JDK-8211425</a>
<br>
<br>
Please follow the parent issue here :
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8202286">https://bugs.openjdk.java.net/browse/JDK-8202286</a>.
<br>
<br>
Webrev:
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00">http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00</a>
<br>
<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"><a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Ekkharbas/8211425/webrev.00"><http://cr.openjdk.java.net/%7Ekkharbas/8211425/webrev.00></a>
<br>
</blockquote>
src/hotspot/share/gc/g1/g1Allocator.inline.hpp
<br>
----------------------------------------------
<br>
100 size_t length = static_cast
<br>
<G1CollectedHeap*>(Universe::heap())-
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
max_reserved_capacity()
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">;
<br>
<br>
You can avoid the cast by using
G1CollectedHeap::heap()
<br>
instead
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
of
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">Universe::heap().
<br>
---
<br>
<br>
src/hotspot/share/gc/g1/g1CollectedHeap.cpp
<br>
-------------------------------------------
<br>
1642 if (AllocateOldGenAt != NULL) {
<br>
1643 _is_hetero_heap = true;
<br>
1644 max_byte_size *= 2;
<br>
1645 }
<br>
<br>
I would like this to be abstracted out of
G1CollectedHeap,
<br>
not entirely sure how but I'm thinking
something like adding
<br>
a G1HeteroCollectorPolicy which answers
correctly on how
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
much
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">memory
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">needs to be reserved
and how big the heap actually is.
<br>
<br>
1668
G1RegionToSpaceMapper::create_heap_mapper(g1_rs,
<br>
1669 g1_rs.size(),
<br>
1670 page_size,
<br>
1671 HeapRegion::GrainBytes,
<br>
1672 1,
<br>
1673 mtJavaHeap);
<br>
<br>
This function could then also make use of
the new policy,
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
something
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">like:
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">create_heap_mapper(g1_rs,
_collector_policy, page_size);
<br>
<br>
1704 if (is_hetero_heap()) {
<br>
1705 _hrm = new
<br>
HeapRegionManagerForHeteroHeap((uint)((max_byte_size
<br>
/ 2) / HeapRegion::GrainBytes /*heap size as
num of regions*/));
<br>
1706 }
<br>
1707 else {
<br>
1708 _hrm = new HeapRegionManager();
<br>
1709 }
<br>
<br>
This code could then become something like:
<br>
_hrm =
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
HeapRegionManager::create_manager(_collector_policy)
<br>
<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">3925
if(g1h->is_hetero_heap()) {
<br>
3926 if(!r->is_old()) {
<br>
....
<br>
3929 r->set_premature_old();
<br>
3930 }
<br>
3931 } else {
<br>
3932 r->set_old();
<br>
3933 }
<br>
<br>
So if I understand this correctly,
premature_old is used when
<br>
we get evac failures and we use it to force
these regions to
<br>
be included in the next Mixed collection.
I'm not sure this
<br>
is needed, in fact I think one of the cool
things with
<br>
nv-dimm is that we can avoid evac failures.
G1 normally needs
<br>
to stop handing out regions to promote to
when there are no
<br>
regions left, but when using nv-dimm the old
regions come
<br>
from another pool and we should
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
be
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">able to avoid this case.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">---
<br>
<br>
src/hotspot/share/gc/g1/g1FullCollector.cpp
<br>
-------------------------------------------
<br>
169 if (_heap->is_hetero_heap())
{
<br>
170 static_cast
<br>
<HeapRegionManagerForHeteroHeap*>(_heap->_hrm)-
<br>
<blockquote type="cite">prepare_for_full_collection_start();
<br>
</blockquote>
171 }
<br>
<br>
Move this to:
<br>
G1CollectedHeap::prepare_heap_for_full_collection()
<br>
<br>
185 if (_heap->is_hetero_heap())
{
<br>
186 static_cast
<br>
<HeapRegionManagerForHeteroHeap*>(_heap->_hrm)-
<br>
<blockquote type="cite">prepare_for_full_collection_end();
<br>
</blockquote>
187 }
<br>
<br>
Move this to:
<br>
G1CollectedHeap::prepare_heap_for_mutators()
<br>
<br>
And if you make the prepare-functions
virtual and not doing
<br>
anything on HeapRegionManager we can avoid
the checks.
<br>
---
<br>
<br>
src/hotspot/share/gc/g1/g1Policy.cpp
<br>
------------------------------------
<br>
223 if(_g1h->is_hetero_heap()
&& (Thread::current()-
<br>
</blockquote>
</blockquote>
</blockquote>
is_VM_thread()
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">||
Heap_lock->owned_by_self())) {
<br>
224 static_cast
<br>
<HeapRegionManagerForHeteroHeap*>(_g1h->hrm())-
<br>
<blockquote type="cite">resize_dram_regions(_young_list_target_length,
<br>
</blockquote>
_g1h->workers());
<br>
225 }
<br>
<br>
Feels like this code should be part of the
<br>
prepare_for_full_collection_end() above, but
the
<br>
_young_list_target_length isn't updated
until right after
<br>
prepare_heap_for_mutators() so you might
need to restructure
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
the
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">code a bit more to
make it fit.
<br>
---
<br>
<br>
src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
<br>
-------------------------------------------------
<br>
Had to add these two includes to make it
compile.
<br>
#include "runtime/java.hpp"
<br>
#include "utilities/formatBuffer.hpp"
<br>
<br>
Please also clean out all code commented
out.
<br>
---
<br>
<br>
src/hotspot/share/gc/g1/heapRegionManager.hpp
<br>
---------------------------------------------
<br>
I agree with Sangheon, please group the
members that should
<br>
be protected and remember to update the
init-list for the
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
constructor.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">Also agree that the
#else on #ifdef ASSERT should be removed.
<br>
---
<br>
<br>
src/hotspot/share/gc/g1/heapRegionSet.cpp
<br>
-----------------------------------------
<br>
240 bool started = false;
<br>
241 while (cur != NULL &&
cur->hrm_index() <= end) {
<br>
242 if (!started &&
cur->hrm_index() >= start) {
<br>
243 started = true;
<br>
244 }
<br>
245 if(started) {
<br>
246 num++;
<br>
247 }
<br>
248 cur = cur->next();
<br>
249 }
<br>
<br>
To me this looks more complex than it is
because of the
<br>
multiple conditions, what do you think
about:
<br>
while (cur != NULL) {
<br>
uint index = cur->hrm_index();
<br>
if (index > end) {
<br>
break;
<br>
} else if (index >= start) {
<br>
num++;
<br>
}
<br>
cur = cur->next();
<br>
}
<br>
---
<br>
<br>
src/hotspot/share/runtime/arguments.cpp
<br>
---------------------------------------
<br>
2072 if(!FLAG_IS_DEFAULT(AllocateHeapAt)
&&
<br>
!FLAG_IS_DEFAULT(AllocateOldGenAt)) {
<br>
2073
jio_fprintf(defaultStream::error_stream(),
<br>
2074 "AllocateHeapAt and
AllocateOldGenAt cannot be
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
used
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">together.\n");
<br>
2075 status = false;
<br>
2076 }
<br>
2077
<br>
2078 if
(!FLAG_IS_DEFAULT(AllocateOldGenAt)
&& (UseSerialGC
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
||
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">UseConcMarkSweepGC ||
UseEpsilonGC || UseZGC)) {
<br>
2079
jio_fprintf(defaultStream::error_stream(),
<br>
2080 "AllocateOldGenAt not supported
for selected GC.\n");
<br>
2081 status = false;
<br>
2082 }
<br>
<br>
This code would fit much better in
GCArguments. There is
<br>
currently no method handling this case but
please add
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
check_args_consistency().
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">You can look at
CompilerConfig::check_args_consistency for
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
inspiration.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">---
<br>
<br>
src/hotspot/share/runtime/globals.hpp
<br>
-------------------------------------
<br>
2612 experimental(uintx,
G1YoungExpansionBufferPerc, 10,
<br>
<br>
Move to g1_globals.hpp and call it
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
G1YoungExpansionBufferPercent.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">---
<br>
<br>
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.*pp
<br>
<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">----------------------------------------------------------
<br>
<br>
Haven't reviewed this code in detail yet but
will do in a
<br>
later
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
review.
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">One thought I have
already is about the name, what do you
<br>
think
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
about:
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">HeterogeneousHeapRegionManager
<br>
---
<br>
<br>
Thanks,
<br>
Stefan
<br>
<br>
<blockquote type="cite">Testing : Passed
tier1_gc and tier1_runtime jtret tests.
<br>
<br>
I added extra
options
<br>
"-XX:+UnlockExperimentalVMOptions -
<br>
</blockquote>
XX:AllocateOldGenAt=/home/Kishor"
<br>
<blockquote type="cite">to each test. There
are some tests which I had to exclude
<br>
since they won't work with this flag.
Example: tests in
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
ConcMarkSweepGC
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">which does not
support this flag, tests requiring
<br>
CompressedOops to be enabled,
<br>
</blockquote>
etc.
<br>
<blockquote type="cite">Thanks
<br>
<br>
Kishor
<br>
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>