RFR (XS): 8023472: C2 optimization breaks with G1

[Vladimir Kozlov]

Thank you, Tony

I added this conversation to the RFE report.
We can get performance from this because we reduce code size by removing 
additional load. It may affect inlining in following compilations 
(InlineSmallCode). I agree that it will be hard to see in most cases.

Thanks,
Vladimir

On 8/22/13 10:59 AM, Tony Printezis wrote:
> Vladimir,
>
> As long as the pre-value is eventually stored on the SATB buffer when
> the thread wakes up, moving it after the store is actually safe. Marking
> will be finalized at a safepoint (remark) and in order for that thread
> to reach the safepoint it will have to wake up and continue, which means
> that it will enqueue the pre-value in the process. (The main reason the
> pre-barrier is before the store is because it has to read the pre-value
> before the actual store for the obvious reasons.)
>
> As you eluded to, this is different to the store / post-barrier
> interaction which has specific ordering requirements (store first,
> post-barrier afterwards).
>
> Regards,
>
> Tony
>
> On 8/22/13 10:33 AM, Vladimir Kozlov wrote:
>> Martin,
>>
>> Thank you for patch. I think change for cmpxchg is fine but I am
>> concern about swapping pre- and post- barriers for xchg.
>> I filed rfe for that:
>>
>> 8023597: Optimize G1 barriers code for unsafe load_store
>>
>> Mikael,
>>
>> What about concurrent marking (not that familiar with GC :) )?
>> The java thread which does this store could be suspended by OS before
>> it executes pre-barrier. GC threads could see new store and dirty_card
>> but there will be no prev_value available.
>>
>> Thanks,
>> Vladimir
>>
>> On 8/22/13 7:31 AM, Mikael Gerdin wrote:
>>>
>>>
>>> On 2013-08-22 15:48, Azeem Jiva wrote:
>>>> Adding the GC dev alias to get their opinion.
>>>>
>>>> --
>>>> Azeem Jiva
>>>> @javawithjiva
>>>>
>>>> On Aug 22, 2013, at 6:34 AM, "Doerr, Martin" wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> we have an addon to your G1 pre-barrier change which reduces
>>>>> its cost.
>>>>> It is possible to get rid of the do_load in inline_unsafe_load_store
>>>>> because we always know the value which gets overwritten.
>>>>>
>>>>> In case of cmpxchg, the only value which might get overwritten
>>>>> is known in advance.
>>>>> In case of xchg, the old value is returned by the load_store.
>>>>> It is allowed to do the barrier after the xchg because there's
>>>>> no safepoint between them.
>>>
>>> I think I understand the change (not that familiar with C2).
>>> I agree that it should be safe to move the barrier as long as there is
>>> no safepoint but I am not familiar enough with C2 to able to verify that
>>> fact.
>>>
>>> But I'm not sure if the possible performance gains are worth the code
>>> complexity and the confusion of having the "pre-barrier" after the
>>> "post-barrier" :)
>>>
>>> /Mikael
>>>
>>>>>
>>>>> Please review
>>>>> http://cr.openjdk.java.net/~goetz/webrevs/opto_g1_barr/
>>>>>
>>>>> Best regards,
>>>>> Martin and Goetz
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: hotspot-compiler-dev-bounces at openjdk.java.net
>>>>> [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of
>>>>> Vladimir Kozlov
>>>>> Sent: Donnerstag, 22. August 2013 02:55
>>>>> To: hotspot compiler
>>>>> Subject: Re: RFR (XS): 8023472: C2 optimization breaks with G1
>>>>>
>>>>> Thanks, Christian
>>>>>
>>>>> On 8/21/13 4:04 PM, Christian Thalinger wrote:
>>>>>>
>>>>>> On Aug 21, 2013, at 3:18 PM, Vladimir Kozlov
>>>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>>>
>>>>>>> http://cr.openjdk.java.net/~kvn/8023472/webrev/
>>>>>>>
>>>>>>> The load of previous value in G1 pre-barrier code (method
>>>>>>> GraphKit::g1_write_barrier_pre()) does not have control. It is from
>>>>>>> the time when G1 code was merged into Hotspot sources. I can only
>>>>>>> speculate that it was done to allow commoning with a preceding
>>>>>>> normal load if it exists and to avoid generation of 2 loads.
>>>>>>> This problem, I think, only affects loads from an array because a
>>>>>>> load from a field should be guarded by NULL check of object.
>>>>>>>
>>>>>>> The fix is to put control on this load to always generate it after
>>>>>>> if (marking) check. I doubt it affect performance much. If there was
>>>>>>> load before the value should be in cache.
>>>>>>
>>>>>> The fix looks good.  Can you at least do a refworkload run to do some
>>>>>> due diligence?
>>>>>
>>>>> Will do it tonight.
>>>>>
>>>>>>>
>>>>>>> Added regression test.
>>>>>>
>>>>>> Could you change the name of the test?  Since we moved away from
>>>>>> using bug ids the test name should more or less explain what it
>>>>>> tests.  I suppose there will be more G1CrashTest's in the future :-)
>>>>>
>>>>> It is external test. I don't want to change it much or its name.
>>>>>
>>>>> Vladimir
>>>>>
>>>>>>
>>>>>> -- Chris
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>
>