[9] RFR (S): 8177963: Parallel GC fails fast when per-thread task log overflows

[Thomas Schatzl]

Hi Kim,

 thanks for your review.

On Tue, 2017-04-04 at 15:35 -0400, Kim Barrett wrote:
> > 
> > On Apr 4, 2017, at 5:48 AM, Thomas Schatzl <thomas.schatzl at oracle.c
> > om> wrote:
> > 
> > Hi all,
> > 
> >   can I have reviews for this small fix that changes a guarantee()
> > in parallel gc when some logging buffer overflows into a warning
> > message about the overflow and (continued) reuse of the last entry
> > of that buffer so that the user then can then rerun with a larger
> > buffer.
> > 
> > Since it is so late in the release cycle I would like to keep the
> > fix simple instead of rewriting the buffer logic, i.e. make it
> > expandable etc. 
> > 
> > This change has been mostly contributed by Ramki from Twitter, as
> > part of JDK-7180571; however we just got this bug from release
> > testing that is a duplicate, and he's unavailable to do an RFR
> > right now, and I want this fixed asap. I intend to put him as
> > author for this change.
> > 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8177963
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8177963/webrev/
> > Testing:
> > local testing, new test, jprt
> > 
> > Thanks,
> >   Thomas
> I'm surprised the type of the "name" argument for add_task_timestamp
> is not "const char*".

Fixed.

> I dislike the casts of GCTaskTimeStampEntries to uint (from uintx),
> introduced so it can be MIN2'd with _time_stamp_index. Could
> GCTaskTimeStampEntries be given a type of uint? Certainly the code
> doesn't really support values that won't fit in uint, since
> _time_stamp_index will overflow.

Done. Initially I thought of that as well, but didn't because I wanted
to keep the changes minimal. Since you are also pointing this out...

> I think it would be helpful for print_task_time_stamps to include
> some indication of overflow in the header it prints.

Done.

> But related to both the cast (for MIN2) and the printing, do we
> really need _time_stamp_index to be the actual number of
> add_task_timestamp calls? Could we instead just have
> add_task_timestamp do nothing (other than warn the first time) when
> the buffer is full?

Continuing to count gives an indication on how large the user needs to
size this buffer without too many retries.

If you think this is a problem (because of overflow after 4M tasks), I
can change that again.

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8177963/webrev.0_to_1 (diff)
http://cr.openjdk.java.net/~tschatzl/8177963/webrev.1 (full)

Testing:
test case, jprt

Thanks,
  Thomas