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

[Bengt Rutisson]

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
>>