RFR(s): 8037112: gc/g1/TestHumongousAllocInitialMark.java caused SIGSEGV

[Mikael Gerdin]

Per,

On Thursday 10 April 2014 15.34.03 Per Liden wrote:
> Bengt reviewed this off-line and had one comment, which was that we
> should also stop G1's string dedup thread if it's running. The dedup
> thread is not affected by the original problem so stopping it is
> strictly not needed, but for completness we still want to do it.
> 
> Updated webrev:
> http://cr.openjdk.java.net/~pliden/8037112/webrev.3/

The change looks good to me.
/Mikael

> 
> Diff against previous webrev:
> http://cr.openjdk.java.net/~pliden/8037112/webrev.3-diff_2vs3/
> 
> /Per
> 
> On 04/08/2014 11:27 AM, Per Liden wrote:
> > Hi,
> > 
> > Here's an updated webrev which fixes the potential dead lock issue I
> > found. I've taken a whole new approach to this problem. Instead of
> > joining/leaving the STS as protection from shutdown I've added a
> > CollectedHeap::stop() which is called in the shutdown paths (in
> > before_exit()) to stop and take down the concurrent threads in a
> > controlled manner. This feel like a much cleaner approach and less error
> > prone as it doesn't require any future log printouts to be guarded by
> > join/leave.
> > 
> > Webrev: http://cr.openjdk.java.net/~pliden/8037112/webrev.2/
> > 
> > cheers,
> > /Per
> > 
> > On 04/04/2014 06:14 PM, Per Liden wrote:
> >> Hmm, I discovered a potential dead lock with this approach, so please
> >> hold reviews on this for now.
> >> 
> >> Sorry!
> >> /Per
> >> 
> >> On 04 Apr 2014, at 14:28, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
> >>> Per,
> >>> 
> >>> On Friday 04 April 2014 14.13.56 Per Liden wrote:
> >>>> Thanks Mikael!
> >>>> 
> >>>> Updated webrev: http://cr.openjdk.java.net/~pliden/8037112/webrev.1/
> >>> 
> >>> Looks good.
> >>> /Mikael
> >>> 
> >>>> /Per
> >>>> 
> >>>> On 04/03/2014 03:59 PM, Mikael Gerdin wrote:
> >>>>> Hi Per,
> >>>>> 
> >>>>> On Thursday 03 April 2014 13.15.12 Per Liden wrote:
> >>>>>> Hi,
> >>>>>> 
> >>>>>> Could I please have this bugfix reviewed.
> >>>>>> 
> >>>>>> Summary: G1's ConcurrentMarkThread tries to use the gclog_or_tty when
> >>>>>> the JVM is about to exit and the gclog_or_tty instance has been
> >>>>>> destroyed. To prevent this, the ConcurrentMarkThread needs to join
> >>>>>> the
> >>>>>> SuspendibleThreadSet before accessing gclog_or_tty.
> >>>>>> 
> >>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8037112
> >>>>>> Webrev: http://cr.openjdk.java.net/~pliden/8037112/webrev.0/
> >>>>> 
> >>>>> While I realize that this code is still semantically correct I
> >>>>> think it
> >>>>> reads a bit strange. In my mind if we have a
> >>>>> if (...) {} else if (...) {} else {} block the checks in the "if" and
> >>>>> "else
> >>>>> if" should usually somehow related checks.
> >>>>> 
> >>>>>   282       if (!cm()->has_aborted()) {
> >>>>>   283         g1_policy->record_concurrent_mark_cleanup_completed();
> >>>>>   284       } else if (G1Log::fine()) {
> >>>>>   285         gclog_or_tty->date_stamp(PrintGCDateStamps);
> >>>>>   286         gclog_or_tty->stamp(PrintGCTimeStamps);
> >>>>>   287         gclog_or_tty->print_cr("[GC concurrent-mark-abort]");
> >>>>>   288       }
> >>>>> 
> >>>>> I would prefer if it was structured the other way around:
> >>>>> if (cm()->has_aborted()) {
> >>>>> 
> >>>>>     if (G1Log::fine()) { ... }
> >>>>> 
> >>>>> else {
> >>>>> 
> >>>>>     g1_policy->record_concurrent_mark_cleanup_completed();
> >>>>> 
> >>>>> }
> >>>>> 
> >>>>> The rest of the change looks good.
> >>>>> 
> >>>>> /Mikael
> >>>>> 
> >>>>>> Thanks!
> >>>>>> /Per
> >>>>>> 
> >>>>>> Btw, I created a separate RFE, JDK-8039147, to do a little bit of
> >>>>>> cleanup of the SuspendibleThreadSet and how it's used. Will send that
> >>>>>> out as separate review request shortly.