Review request (S): 7132311 G1: assert((s == klass->oop_size(this)) || (Universe::heap()->is_gc_active() && ((is_typeArray()...

[Tony Printezis]

Bengt,

Thanks for fixing this and for taking into account my previous 
suggestions. The only very minor change I'd recommend is to replace the 
"alloc size" string in both ergo_verbose calls with "allocation request" 
given that this is what it is called in other places:

       ergo_verbose1(ErgoHeapSizing,
                     "attempt heap expansion",
                     ergo_format_reason("humongous allocation request 
failed")
                     ergo_format_byte("allocation request"),
                     word_size * HeapWordSize);

So, it'd be nice if we were consistent. Apart from that: ship it!

Also, I really liked Stefan's suggestion to do the GC before the 
allocation. It simplified the fix quite a lot...

Tony

On 01/23/2012 01:06 PM, Bengt Rutisson wrote:
>
> Updated webrev based on comments from Tony below:
> http://cr.openjdk.java.net/~brutisso/7132311/webrev.03/
>
> Thanks Stefan and Tony for the quick reviews. As soon as JPRT is back 
> online I will try to push this.
>
> Bengt
>
>
> On 2012-01-23 16:06, Tony Printezis wrote:
>> Bengt,
>>
>> g1CollectedHeap.cpp:
>>
>> 1128     if (result != NULL&&  
>> g1_policy()->need_to_start_conc_mark("STW humongous allocation")) {
>> 1129       g1_policy()->set_initiate_conc_mark_if_possible();
>> 1130     }
>> 1131     return result;
>>
>> Should you maybe leave this in (it should be benign if the cycle is 
>> already in progress or is about to start)? Imagine the following 
>> scenario: a hum allocation is attempted, the old gen + hum allocation 
>> size is just under the threshold, the hum allocation fails 
>> concurrently (not enough contiguous regions to satisfy it), we do a 
>> young GC to satisfy it, at the end of the young GC the old gen 
>> (including the newly-promoted objects) + hum allocation size is over 
>> the threshold (and maybe by a large amount if we promoted a lot). I 
>> know it's a bit of a stretch....
>>
>> g1CollectorPolicy.cpp:
>>
>> 1142   if (_g1->mark_in_progress()) {
>>
>> (As discussed) Please change this to cmThread()->during_cycle().
>>
>> 1149   size_t total_estimated_bytes = cur_used_bytes + 
>> alloc_word_size * BytesPerWord
>>
>> What's the different between BytesPerWord and HeapWordSize? I 
>> generally use the latter.
>>
>> Also, thanks for updating the ergo_verbose entry points. However, 
>> alloc_word_size is in words and you will print it as bytes (and it's 
>> good to be consistent with the units anyway, given that everything 
>> else is in bytes). Maybe just introduce a local field, 
>> alloc_size_bytes, and use it both the condition as well as the 
>> ergo_verbose output?
>>
>> Nits:
>>
>> g1CollectedHeap.cpp:
>>
>> 1056       result = humongous_obj_allocate(word_size);
>> 1057
>> 1058       if (result != NULL) {
>> 1059         return result;
>> 1060       }
>>
>> Can you delete line 1057. In the code the NULL test is generally on 
>> the line after the assignment (at least, it should be in most of the 
>> slow path allocation code).
>>
>> g1CollectorPolicy.hpp
>>
>> 802   bool need_to_start_conc_mark(const char* source, size_t 
>> alloc_word_size);
>>
>> I'd give alloc_word_size a 0 default value to avoid passing 0 when a 
>> size is not needed.
>>
>>
>>
>> On 1/23/2012 7:21 AM, Bengt Rutisson wrote:
>>>
>>> Hi all,
>>>
>>> Can I have one more review for this change, please? Stefan has 
>>> already looked at it.
>>>
>>> http://cr.openjdk.java.net/~brutisso/7132311/webrev.02/
>>>
>>> The idea is that we move the check for whether or not we should 
>>> initiate a marking cycle to before we allocate a humongous object. 
>>> This way we can ignore the issue with uninitialized memory.
>>>
>>> Thanks,
>>> Bengt
>>>
>