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

[Poonam Bajaj]

Hello Mikael,

Thanks for your review. Here's the updated webrev:
http://cr.openjdk.java.net/~poonam/8044406/webrev.01/

regards,
Poonam


On 8/18/2014 2:02 PM, Mikael Gerdin wrote:
> 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
>>