RFR(M): 8073866: Fix for 8064703 is not sufficient

[Roland Westrelin]

>>> About store after allocation check. I don't understand next code:
>>> 
>>> 4852   if (saved_jvms == NULL && alloc != NULL) {
>>> 4853     // We're not emitting the guards, see if we have a tightly
>>> 4854     // allocation now that we've done the null check
>>> 4855     alloc = tightly_coupled_allocation(dest, NULL);
>>> 
>>> I thought we should NULLify alloc if there is store.
>>> 
>>> Also (saved_jvms != NULL) in next checks:
>>> 
>>> 4874   if ((!has_src || !has_dest) && (alloc == NULL || saved_jvms != NULL)) {
>>> 
>>> 4919   if (has_src && has_dest && (alloc == NULL || saved_jvms != NULL)) {
>>> 
>>> It looks like it negates alloc == NULL check since saved_jvms != NULL is true only when alloc != NULL. So the guarded code is executed regardless alloc value. Or I may be missing something
>> 
>> if alloc == NULL we don’t have to worry about guards so we can emit all of them
> 
> yes
> 
>> if saved_jvms != NULL (then alloc != NULL) then we can emit all guards
> 
> Because you will move allocation. Right?

Yes.

>> if saved_jvms == NULL and alloc != NULL, we don’t emit any guard but the arraycopy node could still take advantage of a tightly allocated allocation. The null check is mandatory and if it resulted in an uncommon trap then we don’t have a tightly coupled allocation. That’s why tightly_coupled_allocation() is called again to make sure it takes the null check into account.
> 
> So we don't generate guards here (validated = false) but we generated them later during macro expansion in generate_arraycopy(). Right?

Yes.

> Can you instead create boolean local which is used in this checks and add comments explaining when guards can be emitted 
> and when not and why it is okey to not emit them here.

Ok.

> BTW, arraycopy_move_allocation_here() should check alloc != NULL

Ok.

Roland.

> 
> Vladimir
> 
>> 
>> Roland.
>> 
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>> On 3/12/15 9:17 AM, Roland Westrelin wrote:
>>>> http://cr.openjdk.java.net/~roland/8073866/webrev.00/
>>>> 
>>>> The fix for:
>>>> https://bugs.openjdk.java.net/browse/JDK-8064703
>>>> causes reexecution of an allocation in case we deoptimize between a tightly coupled allocation and an arraycopy so an uninitialized array is not seen in the interpreter. That change causes 2 problems:
>>>> 
>>>> 1) as in the test case in the webrev above, it could cause re-execution of side effects and so be visible from the application. It could even cause incorrect execution.
>>>> 2) it leaves an uninitialized array in the heap. Not all GCs are robust enough to handle that.
>>>> 
>>>> The fix for 1) is to check for no store after the allocation. I verified that restricting the allocations to those not followed by stores don’t cause the performance regression observed in:
>>>> https://bugs.openjdk.java.net/browse/JDK-8060252
>>>> to come back (a regression that happened when tightly coupled allocations were disabled entirely by mistake).
>>>> 
>>>> The fix I propose for 2) is to move the allocation from before the guards to after the guards. Allocations considered tightly coupled follow a pattern that allows that. All other fixes I considered (doing array initialization before the uncommon traps on the slow path, doing array initialization in the uncommon trap runtime code) seemed uglier to me.
>>>> 
>>>> The change TestArrayCopyNoInitDeopt.java guarantees the test passes with -Xcomp and tiered enabled. -Xmixed after -Xcomp on the command line when tiered is enabled doesn’t entirely undo the effect of -Xcomp.
>>>> 
>>>> Roland.
>>>> 
>>