Request for review: 8044406: JVM crash with JDK8 (build 1.8.0-b132) with G1 GC

[Mikael Gerdin]

Hi Poonam,

On 2014-08-17 02:54, Poonam Bajaj wrote:
> Hi,
>
> Could I have reviews for the fix for a G1 GC crash:
>
> 8044406 <https://bugs.openjdk.java.net/browse/JDK-8044406>: JVM crash
> with JDK8 (build 1.8.0-b132) with G1 GC
> Webrev: http://cr.openjdk.java.net/~poonam/8044406/webrev.00/webrev/

7020     // Determine how far we are from the next card boundary. If it 
is smaller than
7021     // the minimum object size we can allocate into, expand into 
the next card.
7022     size_t top = cur->used();
7023     size_t aligned_top = align_size_up_(top, 
G1BlockOffsetSharedArray::N_bytes);

It seems a bit odd to me to do this calculation on the sizes instead of 
the addresses in the region. Also, the naming is confusing since "top" 
almost always refers to the topmost allocated address in a particular 
memory range.
Can you change the above to
HeapWord* top = cur->top();
HeapWord* aligned_top = align_ptr_up...


7024
7025     size_t to_allocate_words = (aligned_top - top) / HeapWordSize;

The subtraction should be replaced by a call to pointer_delta if you 
change the types of top and aligned_top to pointers.

7026     if ((cur->top() + to_allocate_words) != cur->end() && // Not 
completely filled yet.
7027         (to_allocate_words != 0) && // Not at card boundary.
7028         (to_allocate_words <= CollectedHeap::min_fill_size())) {
7029       // Cannot fit any object into the area to the next card 
boundary. Simply create
7030       // an object that fits a single object.
7031       to_allocate_words += CollectedHeap::min_fill_size();
7032     }

Generally, if you feel the need to explain each condition in a 
three-clause if statement then maybe it's a good idea to rephrase it in 
some way. Maybe use a MAX2-expression to get above the minimum object 
size and move the "to_allocate_words != 0" check to encompass the entire 
filling logic?

7033
7034     if (to_allocate_words != 0) {
7035       HeapWord* dummy = 
G1AllocRegion::attempt_allocation(to_allocate_words, true /* bot_updates 
*/);

You don't need the G1AllocRegion scope prefix here, attempt_allocation 
is in the current scope.

7036       CollectedHeap::fill_with_object(dummy, to_allocate_words);
7037     }

/Mikael

>
> The crash happens with the following stack trace:
> Stack: [0x00007fd435a1f000,0x00007fd435b20000], sp=0x00007fd435b1bc30,
> free space=1011k
> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code,
> C=native code)
> V  [libjvm.so+0x541261]
> G1BlockOffsetArray::forward_to_block_containing_addr_slow(HeapWord*,
> HeapWord*, void const*)+0xf1
> V  [libjvm.so+0x959e54] DirtyCardToOopClosure::do_MemRegion(MemRegion)+0x64
> V  [libjvm.so+0x56d2a4] ScanRSClosure::doHeapRegion(HeapRegion*)+0x374
> V  [libjvm.so+0x542dd0]
> G1CollectedHeap::collection_set_iterate_from(HeapRegion*,
> HeapRegionClosure*)+0x60
> V  [libjvm.so+0x56c08c]  G1RemSet::scanRS(OopsInHeapRegionClosure*,
> CodeBlobToOopClosure*, int)+0xdc
> V  [libjvm.so+0x56c4d5]
> G1RemSet::oops_into_collection_set_do(OopsInHeapRegionClosure*,
> CodeBlobToOopClosure*, int)+0x145
> V  [libjvm.so+0x549ef4] G1CollectedHeap::g1_process_strong_roots(bool,
> SharedHeap::ScanningOption, OopClosure*, OopsInHeapRegionClosure*,
> G1KlassScanClosure*, int)+0x224
> V  [libjvm.so+0x558a88]  G1ParTask::work(unsigned int)+0xb88
> V  [libjvm.so+0xa4a9ff]  GangWorker::loop()+0xcf
> V  [libjvm.so+0x8a0058]  java_start(Thread*)+0x108
>
> Here, the GC thread crashes while scanning the RemSet (part of the
> non-CSet regions). And it happens to scan a region to which another
> thread in G1ParEvacuateFollowersClosure is copying contents to, and this
> region was found out to be an Old GC alloc region.
>
> This change makes sure that we fill up the last card of the Old GC alloc
> region that was being allocated into, with a dummy object so that it
> does not get scanned by the remset scanning GC threads.
>
> This change applies cleanly to 8u and 7u repos.
>
> Thanks to Thomas Schatzl for his help in investigating the crash and
> suggesting this solution.
>
> Testing:
> - JPRT
> - Testing by Coherence QA
>
> Thanks,
> Poonam
>