RFR - Avoid G1 write barriers on newly allocated objects

[Staffan Friberg]

Anyone available to do a second review?

Thanks,
Staffan

On 09/16/2014 03:16 PM, Vladimir Kozlov wrote:
> Looks fine but you need to add '.' at the end of statements in the 
> comment you modified. No need for re-review.
>
> Thanks,
> Vladimir
>
> On 9/16/14 2:41 PM, Staffan Friberg wrote:
>> Thanks Vladimir,
>>
>> Updated with your suggestions. Added a small comment in
>> g1_can_remove_pre_barrier method since the alloc == NULL check is quite
>> a bit earlier.
>>
>> New webrev, http://cr.openjdk.java.net/~sfriberg/8057737/webrev.03
>>
>> Cheers,
>> Staffan
>>
>>
>> On 09/15/2014 05:55 PM, Vladimir Kozlov wrote:
>>> You need only check (alloc == st_alloc) because alloc != NULL at this
>>> point:
>>>
>>> +    if (st_alloc != NULL && alloc == st_alloc) {
>>>
>>>
>>> You can clean up g1_can_remove_pre_barrier() too (remove st_alloc ==
>>> NULL check).
>>>
>>> I would also reverse the last check there because you bailout from
>>> loop anyway:
>>>
>>> +        if (captured_store == NULL || captured_store ==
>>> st_init->zero_memory()) {
>>> +          return true;
>>> +        }
>>> +      }
>>> +    }
>>> +
>>> +    // Unless there is an explicit 'continue', we must bail out here,
>>>
>>>
>>> Vladimir
>>>
>>> On 9/15/14 3:31 PM, Staffan Friberg wrote:
>>>> Hi Vladimir,
>>>>
>>>> Uploaded a new webrev with fixes to the below issues,
>>>> http://cr.openjdk.java.net/~sfriberg/8057737/webrev.02
>>>>
>>>> Still removes expected barriers.
>>>>
>>>> Thanks,
>>>> Staffan
>>>>
>>>> On 09/12/2014 04:15 PM, Vladimir Kozlov wrote:
>>>>> Fix indention in 2 checks:
>>>>>
>>>>> +    if (use_ReduceInitialCardMarks() &&
>>>>> +            g1_can_remove_*_barrier(&_gvn, adr, bt, alias_idx)) {
>>>>>
>>>>> Typo 'iff' in both comments:
>>>>>
>>>>> + * Returns true iff the barrier can be removed
>>>>>
>>>>> In g1_can_remove_post_barrier() use:
>>>>>
>>>>> +  // Start search from Store node
>>>>> +  Node* ctrl = store->in(MemNode::Control);
>>>>>
>>>>> Control edge can't point to an other Store so you should not check it
>>>>> inside loop.
>>>>> As result you don't need loop.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 9/12/14 2:26 PM, Staffan Friberg wrote:
>>>>>> Hi Mikael,
>>>>>>
>>>>>> I updated the documentation, and uploaded a new webrev,
>>>>>> http://cr.openjdk.java.net/~sfriberg/8057737/webrev.01
>>>>>>
>>>>>> Thanks for agreeing to sponsor, and the good idea about pushing to
>>>>>> hs-gc for extra coverage.
>>>>>>
>>>>>> Thanks,
>>>>>> Staffan
>>>>>>
>>>>>> On 09/09/2014 07:31 AM, Mikael Gerdin wrote:
>>>>>>> Hi Staffan,
>>>>>>>
>>>>>>> On Friday 05 September 2014 16.31.44 Staffan Friberg wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Copying both the compiler and GC alias as the optimization 
>>>>>>>> involves
>>>>>>>> both
>>>>>>>> groups.
>>>>>>>>
>>>>>>>> This optimization aims to avoid generating G1 barriers for newly
>>>>>>>> allocated objects where the compiler can prove that the object
>>>>>>>> has not
>>>>>>>> been written to earlier and there is no safepoint between the
>>>>>>>> allocation
>>>>>>>> and the write. The bug has some further details and microbenchmark
>>>>>>>> result. The new code has fairly extensive comments about the
>>>>>>>> optimization.
>>>>>>>>
>>>>>>>> It would be great if the GC team can help validate that the
>>>>>>>> premise of
>>>>>>>> the optimization is correct as well.
>>>>>>>>
>>>>>>>> webrev: http://cr.openjdk.java.net/~sfriberg/8057737/webrev/
>>>>>>> I mostly read through the comments since I can't decode what the IR
>>>>>>> transforms
>>>>>>> intend to do :)
>>>>>>>
>>>>>>> We usually stick to the terms "old generation" and "young
>>>>>>> generation" instead
>>>>>>> of "Old Space" and "Young Space".
>>>>>>>
>>>>>>> "G1 also requires to keep track of objects between different
>>>>>>> + * regions to enable evacuation of old regions"
>>>>>>> should probably be.
>>>>>>> .."to keep track of references between different regions"
>>>>>>>
>>>>>>> I leave it to the compiler team to review the actual code changes.
>>>>>>>
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8057737
>>>>>>>>
>>>>>>>> I would also need a sponsor for this change if it passes review.
>>>>>>> I'll push this to hs-gc after review since we have more G1 test
>>>>>>> coverage on
>>>>>>> hs-gc.
>>>>>>>
>>>>>>> /Mikael
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Staffan
>>>>>>
>>>>
>>