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

[Vladimir Kozlov]

On 3/13/15 4:23 AM, Roland Westrelin wrote:
> Thanks for looking at this, Vladimir.
>
>> An other approach would be to replace uncommon traps from safe guards with a slow path which use ClearArray node to zero allocation and then go to arraycopy. So you don't need to move arraycopy and reset jvm state. It may perform slower then code with uncommon traps (we need to check) then we go with your implementation.
>
> Wouldn’t that defeat the purpose of the recent arraycopy changes?
> We want allocation + arraycopy to be turned into allocation + loads/stores so if the allocation doesn’t escape we can optimize out allocation + arraycopy entirely. But if we add a slow path then the allocation has new uses and it becomes harder to eliminate the allocation + arraycopy?

For small allocations ClearArray is converted into stores too. Please, try a small test case. It should be folded 
nicely. Anyway a test is needed to see if it really prevents elimination.

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