RFR: 8069367: assert(_nextMarkBitMap->isMarked((HeapWord*) obj)) failed

[Bengt Rutisson]

On 2015-03-30 03:28, Kim Barrett wrote:
> On Mar 28, 2015, at 12:31 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> On Mar 27, 2015, at 4:22 PM, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:
>>> That's my only question and it is for my education so you can
>>> consider this a complete 2nd review.
>> Well, there may be further changes.  Thomas and Bengt wondered if excluding typeArrays
>> allocated before the start of the in-progress concurrent mark might matter.  I’ve been looking
>> at a way to still allow that.
>>
>> Though maybe the version that has been reviewed is good enough, and the possible further
>> improvement can be done as followup work?  Bengt and Thomas?
> Another round on this.
>
> In the previous version we disallowed eager reclaim of any humongous
> objects that were allocated prior to the current snapshot when
> concurrent marking was in progress.  As noted, there was some concern
> (private email with Thomas and Bengt) that this would result in more
> headroom being needed because some humongous objects that were
> previously reclaimed now would not be.
>
> This time we allow pre-snapshot humongous typeArrays to be candidates
> for reclaiming.  However, we now ensure that such objects won't ever
> be added to the mark stack.  We accomplish this by filtering out all
> typeArrays from the mark stack; rather than pushing them we
> immediately perform the "scan", which for typeArray objects amounts to
> just doing some bookkeeping and checking task limits.
>
> The addition of typeArray filtering in front of the mark stack is
> rather far away from the place that is requiring it.  I'm not entirely
> happy with this coupling, and would be OK with not going this extra
> step.
>
> The cost of the pre-filtering of the mark stack is small, and some of
> it may be recovered by avoiding the cost of moving the filtered
> objects through the mark stack, and by doing less work on the filtered
> objects than would otherwise be needed. Aurora GC performance tests
> show mostly insignificant differences, with a few small improvements
> compared to the baseline that has bug 8069367.

I like this approach. I think it makes sense even without the humongous 
reclamation issues to handle type arrays specially.

> [While I was at it, I simplified the conditionalization of
> deal_with_references for _CHECK_BOTH_FINGERS_, eliminating some code
> duplication.]

Personally I would prefer to do a small cleanup fix before your change 
to simply remove _CHECK_BOTH_FINGERS_. It has been in the code since 
2011 (JDK-7046558: G1: concurrent marking optimizations) and as I 
understand it it was mostly added because there was not time to do 
enough performance testing at the time. I doubt that anyone will pick up 
the performance work here, so I'd rather remove the code.

> CR:
> https://bugs.openjdk.java.net/browse/JDK-8069367
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8069367/webrev.03/


A couple of questions about this code in concurrentMark.inline.hpp:


  300           if (_CHECK_BOTH_FINGERS_ && _finger != NULL && objAddr < 
_finger) {
  301             if (obj->is_typeArray()) {
  302               // Immediately process arrays of binary data, rather
  303               // than pushing on the mark stack.  This keeps us from
  304               // adding humongous objects to the mark stack that might
  305               // be reclaimed before the entry is processed - see
  306               // G1EagerReclaimHumongousPreSnapshotTypeArrays. The
  307               // cost of the additional type test is mitigated by
  308               // avoiding a trip through the mark stack, and by only
  309               // doing bookkeeping update and avoiding the actual scan
  310               // of the object - a typeArray contains no references,
  311               // and the metadata is built-in.
  312               process_grey_object<false>(obj);
  313             } else {
  314               if (_cm->verbose_high()) {
  315                 gclog_or_tty->print_cr(
  316                   "[%u] below the local finger (" PTR_FORMAT "), 
pushing " PTR_FORMAT,
  317                   _worker_id, p2i(_finger), p2i(objAddr));
  318               }
  319               push(obj);
  320             }


It looks to me like the logging (lines 314-318) is relevant for type 
arrays too. Can we move it up before line 301? Possibly changing the 
wording from "pushing" to something more generic - maybe simply "obj".

I think the comment (lines 302-311) is good but it takes up a lot of 
space inside the deal_with_reference() method. How about breaking lines 
301-320 out into its own method? (Keeping the logging that I mentioned 
in the previous paragraph inside deal_with_reference().)


I'm not sure I understand the changes in the g1CollectedHeap.* files. In 
particular the changes to RegisterHumongousWithInCSetFastTestClosure:

http://cr.openjdk.java.net/~kbarrett/8069367/webrev.03/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.frames.html

It looks like the G1EagerReclaimHumongousPreSnapshotTypeArrays flag 
controls if you want to use the previous version (don't reclaim objects 
that may be part of the SATB snapshot) and the new version (don't push 
type arrays on the mark stack). Is this debugging code that was left or 
is your intention that we should have both versions in the code?

Thanks,
Bengt


>
> Incremental webrev:
> http://cr.openjdk.java.net/~kbarrett/8069367/webrev.03.incr/
>
> Testing:
> Hand testing
> JPRT
> Aurora Ad-hoc GC Nightly using G1
> Aurora GC perf test using G1.
>