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

Vladimir Kozlov vladimir.kozlov at oracle.com
Mon Mar 16 21:38:29 UTC 2015

On 3/16/15 3:45 AM, Roland Westrelin wrote:

About store after allocation check. I don't understand next code:

4852 if (savedjvms == 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 = tightlycoupledallocation(dest, NULL); I thought we should NULLify alloc if there is store. Also (savedjvms != NULL) in next checks: 4874 if ((!hassrc || !hasdest) && (alloc == NULL || savedjvms != NULL)) { 4919 if (hassrc && hasdest && (alloc == NULL || savedjvms != NULL)) { It looks like it negates alloc == NULL check since savedjvms != 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 savedjvms != NULL (then alloc != NULL) then we can emit all guards Because you will move allocation. Right? Yes. if savedjvms == 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 tightlycoupledallocation() 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 generatearraycopy(). 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, arraycopymoveallocationhere() 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.

More information about the hotspot-compiler-dev mailing list