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

[Vladimir Kozlov]

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.

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

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