RFE: 8023597: Optimize G1 barriers code for unsafe load_store

[Vladimir Kozlov]

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