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

[Tony Printezis]

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" <martin.doerr at sap.com
>>> <mailto:martin.doerr at sap.com>> 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
>>>>>>
>>>

-- 
Tony Printezis | Staff Software Engineer | Twitter

@TonyPrintezis
tprintezis at twitter.com