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

[Vladimir Kozlov]

On 3/16/15 3:45 AM, Roland Westrelin wrote:
>
>>>> 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.

Thanks.
It should be != null since saved_jvms != null. But to have explicit 
check is better for understanding.

Thanks,
Vladimir

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