<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Thomas,<br>
<br>
<div class="moz-cite-prefix">On 03/13/2018 06:52 AM, Thomas Schatzl
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1520949153.3100.27.camel@oracle.com">
<pre wrap="">Hi Stefan,
On Wed, 2018-03-07 at 11:12 +0100, Stefan Johansson wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Thanks Thomas,
This looks good to me, reviewed.
</pre>
</blockquote>
<pre wrap="">
thanks for your review; unfortunately rebasing the changes to latest
caused some additional work.
In this particular case, contents of g1RegionMarkStatsCache had to be
distributed to .cpp and .inline.hpp files (recent .inline.hpp include
fixes), and I preemptively removed the use of one VALUE_OBJ_CLASS_SPEC.
Webrevs:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8197850/webrev.1_to_2/index.html">http://cr.openjdk.java.net/~tschatzl/8197850/webrev.1_to_2/index.html</a>
(diff)
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8197850/webrev.2/index.html">http://cr.openjdk.java.net/~tschatzl/8197850/webrev.2/index.html</a> (full)</pre>
</blockquote>
Looks good, I have several minor comments.<br>
<br>
===============================<br>
src/hotspot/share/gc/g1/g1CardLiveData.cpp<br>
<pre><span class="new">292 assert(!hr->is_old() || marked_bytes == (_cm->liveness(hr->hrm_index()) * HeapWordSize),</span></pre>
Don't we need to print assert message for hr->is_old()?<br>
<br>
===============================<br>
src/hotspot/share/gc/g1/g1CollectedHeap.cpp<br>
<pre><span class="changed">4215 // Any worker id is fine here as we are in the VM thread and single-threaded.</span></pre>
<br>
Would be better to add 'valid'?<br>
// Any <u>valid</u> worker id ...<br>
<br>
===============================<br>
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp<br>
<pre><span class="changed"></span></pre>
<pre><span class="new">1840 log_debug(gc, stats)("Mark stats cache hits " SIZE_FORMAT " misses " SIZE_FORMAT " ratio %1.3lf",</span></pre>
gc+stats is not registered at logPrefix.hpp, so currently GC ID is
not printed. I guess this is not intensional?<br>
<br>
===============================<br>
src/hotspot/share/gc/g1/g1RegionMarkStatsCache.hpp<br>
<pre>25 #ifndef SHARE_VM_GC_G1_G1REGIONMARKSTATSCACHE_HPP</pre>
I'm okay leaving as is but we don't have 'VM' directory anymore. At
some point we would want to clean up these stuffs at once?<br>
<br>
<pre> 61 // logical and.</pre>
This seems incomplete sentence?<br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<blockquote type="cite"
cite="mid:1520949153.3100.27.camel@oracle.com">
<pre wrap="">
Testing:
hs-tier 1-5
Thanks,
Thomas
</pre>
<blockquote type="cite">
<pre wrap="">
Stefan
On 2018-03-07 09:48, Thomas Schatzl wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi,
On Tue, 2018-03-06 at 15:03 +0100, Stefan Johansson wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On 2018-03-06 14:33, Thomas Schatzl wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On Tue, 2018-03-06 at 13:31 +0100, Stefan Johansson wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi Thomas,
On 2018-03-05 15:41, Thomas Schatzl wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi all,
...
CR:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8197850">https://bugs.openjdk.java.net/browse/JDK-8197850</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8197850/webrev/index.h">http://cr.openjdk.java.net/~tschatzl/8197850/webrev/index.h</a>
tml
</pre>
</blockquote>
<pre wrap="">
Look good, just some minor things.
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
394 _region_mark_stats(NEW_C_HEAP_ARRAY(G1RegionMarkStats,
max_regions, mtGC))
max_regions is passed into the constructor but everywhere
else in
G1ConcurrentMark we use g1h->max_regions(). I would consider
either
calling g1h->max_regions() in the constructor too, or save
the
value
in a member and use it everywhere.
</pre>
</blockquote>
<pre wrap="">
Done using option 1), use g1h->max_regions() everywhere.
</pre>
</blockquote>
<pre wrap="">
Thanks.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">---
src/hotspot/share/gc/g1/g1_globals.hpp
262 experimental(size_t, G1RegionMarkStatsCacheSize,
1024,
...
265 range(128, (max_juint / 2) + 1)
Do we need this flag or should we scale it against the number
of
regions? If we want to keep it I think we should have a
somewhat
more restrictive range.
</pre>
</blockquote>
<pre wrap="">
I would not want this to scale according to the number of heap
regions,
because that is going to have a significant impact on pause
times
when
all of the thread's caches are evicted (The O(#regions) part).
</pre>
</blockquote>
<pre wrap="">
True, just a thought since 1024 is roughly half the number
regions
we aim for maybe this could be used together with a reasonable
upper
bound.
</pre>
<blockquote type="cite">
<pre wrap="">The flag has only been added (rather late, admittedly) to fix
potential issues with marking time with really large heaps with
tens of thousands of regions.
Let me do some more measurements, I will get back to you with
them
before giving you a new webrev.
</pre>
</blockquote>
<pre wrap="">
Sound good.
</pre>
</blockquote>
<pre wrap="">
I did some measurements with 100k regions, and I think we can just
remove the flag. There is always logging available (gc+mark+stats)
that
shows cache hit ratio to diagnose issues.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">---
src/hotspot/share/gc/g1/g1RegionMarkStatsCache.hpp
In G1RegionMarkStatsCacheEntry _region_idx is set to 0 when
clearing. I don't see a problem with it right now, but it
feels a
bit wrong since index 0 is a valid region index. Maybe we
could
use another cleared marker.
</pre>
</blockquote>
<pre wrap="">
Unfortunately there is none (one could use something like -1,
but
that one is valid too), and it seems a waste of memory to
reserve
more space for it.
I see your point that this is a bit ugly, but there should
never be
any issue because of the initialization of the value part of an
entry with zero. It simply has no impact on totals if you add
zero.
The code could initialize the cache with region indices from 0
to
cache size - 1, which are valid values given the hash function,
what do you think?
However that would slow down clearing a little (the compiler
will
just compile it to a memset() in the loop right now).
</pre>
</blockquote>
<pre wrap="">
You are correct that -1 is a valid index, but we seldom see heaps
with so many regions. I'm fine with leaving it as is, if you
dislike
using -1 and if it will slow down clearing.
</pre>
</blockquote>
<pre wrap="">
Just a bit. It seems as bad to use -1 or another random value, so I
kept it.
Webrevs:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8197850/webrev.0_to_1/index.ht">http://cr.openjdk.java.net/~tschatzl/8197850/webrev.0_to_1/index.ht</a>
ml
(diff)
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8197850/webrev.1/index.html">http://cr.openjdk.java.net/~tschatzl/8197850/webrev.1/index.html</a>
(full)
Testing:
This change ran, with some changes to 8180415 due to internal
comments
(I will post in a few minutes) through hs-tier 1-5 over night.
Thanks,
Thomas
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>