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

[Roland Westrelin]

Thanks Vladimir.

Here is a new webrev with more comments, a boolean to indicate when we can emit the guards and an assert in arraycopy_move_allocation_here() that alloc != NULL.

http://cr.openjdk.java.net/~roland/8073866/webrev.01/

Roland.

> On Mar 16, 2015, at 10:35 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> On 3/16/15 4:24 AM, Roland Westrelin wrote:
>>> One more question. Instead of moving allocation, can we insert checks before it instead?
>>> You still need to reset jvmstate for checks but you may need less code than you currently have in arraycopy_move_allocation_here().
>> 
>> The allocation could be followed by a load that would in turn be an argument to arraycopy. The allocation could still be considered tightly coupled. Then the guards would need the result of the load. So not only would we need the guards before the allocation we would also need to move the load before. So it felt simpler to me to move the allocation down that the guards up.
> 
> Yes, loads could be problem in my suggestion.
> 
>> 
>>> 
>>> Also if we have allocation we don't need its null check:
>>> dest = null_check(dest, T_ARRAY);
>> 
>> Can’t we let c2 figure out that it doesn’t need to emit it rather than make the control flow even harder to follow by adding another test?
> 
> I was suggested that for case if move guards above allocation including null check but we can't check the allocation in such case.
> 
> Okay, lets move allocation.
> 
> Thanks,
> Vladimir
> 
> 
>> 
>> Roland.
>> 
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>>> 
>>>> Or am I missing something?
>>>> 
>>>> We could do:
>>>> 
>>>> if (guards) {
>>>>   // fast path
>>>>   ArrayCopyNode
>>>> } else {
>>>>   // slow path
>>>>   ClearArray
>>>>   uncommon_trap
>>>> }
>>>> 
>>>> That was my initial fix for 8064703 before you suggested trying to reexecute from the allocation which felt prettier. I kind of assumed we wouldn’t want to increase code size for never executed code paths. As the code is today there can be several different uncommon traps for a single arraycopy so we could need several ClearArray nodes.
>>>> 
>>>> Roland.
>>>> 
>>>> 
>>>>> 
>>>>>> The allocation + arraycopy scenario was mentioned for parameter passing in lambda forms. It’s a use case that’s not implemented yet so any performance testing we do now won’t tell us anything about it.
>>>>>> 
>>>>>> 
>>>>>>> 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?
>>>>> 
>>>>>> 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?
>>>>> 
>>>>> 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.
>>>>> 
>>>>> BTW, arraycopy_move_allocation_here() should check alloc != NULL
>>>>> 
>>>>> 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.
>>>> 
>>