RFE: 8023597: Optimize G1 barriers code for unsafe load_store

[Doerr, Martin]

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
>