RFE: 8023597: Optimize G1 barriers code for unsafe load_store

[Vladimir Kozlov]

Martin,

This is good. I will push it.

Thanks,
Vladimir

On 8/28/13 5:46 AM, Doerr, Martin wrote:
> Hi Vladimir,
>
> I see what you mean. It's pointless at the moment, but for the future ...
> Here's the new webrev:
> http://cr.openjdk.java.net/~simonis/webrevs/8023597.v3/
>
> Kind regards,
> Martin
>
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Dienstag, 27. August 2013 19:52
> To: Doerr, Martin
> Cc: Tony Printezis; hotspot compiler; hotspot-gc-dev at openjdk.java.net
> Subject: Re: RFE: 8023597: Optimize G1 barriers code for unsafe load_store
>
> It is good except can_move_pre_barrier() . Can you make it like
> pre_barrier() - out-of-line and with a switch because it is not just G1:
>
>     switch (bs->kind()) {
>       case BarrierSet::G1SATBCT:
>       case BarrierSet::G1SATBCTLogging:
>         return true; // Can move it if no safepoint
>       case BarrierSet::CardTableModRef:
>       case BarrierSet::CardTableExtension:
>       case BarrierSet::ModRef:
>         return true; // There is no pre-barrier
>
>       case BarrierSet::Other:
>       default      :
>         ShouldNotReachHere();
>     }
>     return false;
>
> thanks,
> Vladimir
>
> On 8/27/13 6:23 AM, Doerr, Martin wrote:
>> Vladimir, Tony,
>>
>> thanks for your input and explanations.
>>
>> It makes sense to keep the code more generic. Here's a new webrev:
>> http://cr.openjdk.java.net/~simonis/webrevs/8023597.v2/
>>
>> Please review. Thanks.
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Montag, 26. August 2013 21:30
>> To: Doerr, Martin
>> Cc: Tony Printezis; hotspot compiler; hotspot-gc-dev at openjdk.java.net
>> Subject: Re: RFE: 8023597: Optimize G1 barriers code for unsafe load_store
>>
>> Martin,
>>
>> I think you need to call pre_barrier() anyway and have the NULL pointer
>> check in g1_write_barrier_pre() when pre_val is passed in
>> (do_load==false). Method pre_barrier() is common interface for all
>> collectors and decision about NULL_PTR could be different.
>>
>> For the same reason you can move pre_barrier() for LS_xchg only if used
>> collector allows that. I think you need new method
>> GraphKit::can_move_pre_barrier() or similar.
>>
>> Yes, all current collector allows that but it may be not true in a future.
>>
>>    > Vladimir, are you familiar with what Tony has mentioned wrt. young
>> gen writes?
>>    > Maybe we can optimize out more barriers.
>>
>> Yes, we do that (skipping GC barriers) for oop stored for CMS, see
>> GraphKit::write_barrier_post(). For arraycopy for all collectors (see
>> ReduceInitialCardMarks). And for initializing stores (collected by
>> InitializeNode).
>>
>> For G1 store oops, it is done by generated code, if I understand
>> correctly, by checking in pre-barrier the field satb_mark_queue._active
>> and checking cardmark in post-barrier.
>>
>> Yes, we can improve barriers generation for G1. For example, we have
>> 6816756 RFE.
>>
>> Thanks,
>> Vladimir
>>
>> On 8/26/13 7:13 AM, Doerr, Martin wrote:
>>> Hi all,
>>>
>>> I have created a new webrev:
>>> http://cr.openjdk.java.net/~simonis/webrevs/8023597/
>>>
>>> The comment
>>> "Transformation of a value which could be NULL pointer could be delayed during Parse"
>>> directly above my change seems to explain why the null reference with basic type T_ADDRESS was there.
>>> So I assume we should handle oldval the same way as newval.
>>>
>>> The new webrev elides the pre-barrier in this case in which oldval is a null reference.
>>> Please double-check if it's correct. Jvm2008 derby is running without assertions, now.
>>>
>>> Vladimir, are you familiar with what Tony has mentioned wrt. young gen writes?
>>> Maybe we can optimize out more barriers.
>>>
>>> Best regards,
>>> Martin
>>>
>>>
>>> -----Original Message-----
>>> From: Tony Printezis [mailto:tprintezis at twitter.com]
>>> Sent: Montag, 26. August 2013 14:55
>>> To: Doerr, Martin
>>> Cc: Vladimir Kozlov; hotspot compiler; hotspot-gc-dev at openjdk.java.net
>>> Subject: Re: RFE: 8023597: Optimize G1 barriers code for unsafe load_store
>>>
>>> Martin,
>>>
>>> On 8/26/13 4:52 AM, Doerr, Martin wrote:
>>>>
>>>> Regardless of the type issue, I guess we should not generate any pre-barrier code when C2 can
>>>> determine that oldval is a null reference.
>>>
>>> This is totally correct, the pre-barrier can be elided if the pre-value
>>> is null (no point in enqueueing null!).
>>>
>>> Additionally, the pre-barrier can also be elided for writes in the young
>>> gen. IIRC, there is (or at least used to be!) some code in C2 to elide
>>> card table writes for objects in the young gen (a fast path allocation
>>> is guaranteed to allocate the object in the young gen and the object
>>> cannot move until the next safepoint). So, if it's not done already, you
>>> could take advantage of the same mechanism to also elide pre-barriers
>>> (and post-barriers of course).
>>>
>>> Tony
>>>
>>>
>>>> Regards,
>>>> Martin
>>>>
>>>> -----Original Message-----
>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>> Sent: Samstag, 24. August 2013 02:36
>>>> To: Doerr, Martin
>>>> Cc: hotspot compiler; hotspot-gc-dev at openjdk.java.net
>>>> Subject: Re: RFE: 8023597: Optimize G1 barriers code for unsafe load_store
>>>>
>>>> On 8/23/13 9:09 AM, Doerr, Martin wrote:
>>>>> Hi,
>>>>>
>>>>> I did some more tests with our proposed webrev
>>>>> http://cr.openjdk.java.net/~goetz/webrevs/opto_g1_barr/
>>>>> and it happened that the assertion
>>>>> "assert(pre_val->bottom_type()->basic_type() == T_OBJECT, "or we
>>>>> shouldn't be here");"
>>>>> was firing when running jvm2008 derby on linuxx86_64.
>>>>>
>>>>> What happened is that the C2 was inserting a barrier of kind=LibraryCallKit::LS_cmpxchg,
>>>>> but oldval was a ConP node with bottom_type()->basic_type() = T_ADDRESS.
>>>> What is ConP (oldval->dump())? I am worried that compareAndSwapObject()
>>>> is used not for object.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>> I believe there's nothing logically wrong with the change.
>>>>> Should the assertion be modified or should we filter out cases in which oldval
>>>>> does not contain a valid Oop?
>>>>>
>>>>> I guess some advice from C2 and G1 experts is needed.
>>>>>
>>>>> Best regards,
>>>>> Martin
>>>