RFR: 8159464: DumpHeap.java hits assert in G1 code

[Erik Helin]

On 2016-07-19, Thomas Schatzl wrote:
> Hi,
> 
> On Mon, 2016-07-18 at 15:59 +0200, Erik Helin wrote:
> > Hi all,
> > 
> > this patch fixes an issue with the young gen sizing code in G1. The
> > problem is that the G1 policy uses a binary search to come up with
> > the
> > young gen size, and the context for this binary search might change
> > while it is running. For this particular bug, a concurrent mark cycle
> > ended while the binary search was running. To fix this, we must save
> > a
> > copy of all the state needed for the search.
> > 
> > I've looked through the code and as far as I can see the only data
> > that
> > might change is the collector state (and the during_cm() state in
> > particular). The fix is to save the value of
> > collector_state()->during_cm() before the binary search. The data in
> > G1Analytics is not updated while the young gen sizing code is
> > running.
> > 
> > Bug:
> > https://bugs.openjdk.java.net/browse/JDK-8159464
> > 
> > Patches:
> > I split the patch into two steps to make it a bit easier to review:
> > 1. Add const to a lot of variables to ensure that they don't change:
> >    http://cr.openjdk.java.net/~ehelin/8159464/00/add-const/
> > 2. Move G1DefaultPolicy::predic_will_fit into its own class,
> >    G1YoungLenghtPredictor. The G1YoungLengthPredictor class takes the
> >    needed parameters in its constructor:
> >    http://cr.openjdk.java.net/~ehelin/8159464/00/fix-bug/
> 
>   - G1YoungLengthPredictor needs a VALUE_OBJ_CLASS_SPEC or something
> similar.

Thanks, fixed, below is the incremental patch:

diff -r e19783ebbecf src/share/vm/gc/g1/g1DefaultPolicy.cpp
--- a/src/share/vm/gc/g1/g1DefaultPolicy.cpp    Mon Jul 18 14:05:41 2016 +0200
+++ b/src/share/vm/gc/g1/g1DefaultPolicy.cpp    Thu Jul 21 16:16:47 2016 +0200
@@ -99,3 +99,3 @@
 
-class G1YoungLengthPredictor {
+class G1YoungLengthPredictor VALUE_OBJ_CLASS_SPEC {
   const bool _during_cm;

On 2016-07-19, Thomas Schatzl wrote:
>   - looking at the code, I think using an extra class to hold the
> repeatedly passed parameters is quite nice.

Thanks, I also think it turned out quite well.

On 2016-07-19, Thomas Schatzl wrote:
>   - this is just some question about the impact of the change of
> during_concurrent_mark while we are calculating a new target length. I
> would assume that the resulting young length during mark would at most
> be smaller than the one determined later.
> 
> I.e. at most we would do a young gc a little bit too early, assuming
> that for some reason object copy is a bit more expensive during that
> time? Not sure about the logic for having different costs during and
> ouside marking right now, do you remember?
> 
> Any concerns from you about this?

I don't know why the policy thinks object copying is more expensive
during concurrent mark, most likely this was the case waaaay back but is
no longer true. I filed JDK-8162109 [0].

As to the effect of this patch, we shouldn't notice any difference. This
is most likely the first time we have observed this behavior (otherwise
the assert should have triggered), so we have probably never run into
this situation before :) Remember that this bug only happens when a
concurrent mark cycle finishes during the calculation of the eden length
(not common).

Thanks,
Erik

[0]: https://bugs.openjdk.java.net/browse/JDK-8162109

> Thanks,
>   Thomas