RFC: 6898462: The escape analysis with G1 cause crash assertion src/share/vm/runtime/vframeArray.cpp:94

[Roland Westrelin]

Thanks for the comments. I’ll work on your suggestions.


> On Nov 19, 2014, at 12:19 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> Good :) Really, I almost did not find any serious issues with these changes.
> 
> The only question I have: why not pop all interpreter frames corresponding to deoptimized compiled frame? You are trying to find the top popped frame in vframeArray.cpp. But how we guarantee that frames which are not popped are in correct state after deoptimization?


That was John’s suggestion (in the chat room) or at least how I interpreted it:

"I like the frame-popping idea.  Given a bunch of frames to de-opt with OOME, you only need to pop until there are no more locals to materialize.”

But you’re right that objects A and B could have been scalar replaced. A has a reference to B. B is not reallocated. A is. Then A has a reference to null and some activation we don’t pop could try to access the non allocated B through A which would cause a NPE. So I agree that we should pop all frames. What do you think, John?

Roland.

> 
> deoptimization.cpp:
> 
> I think next assert should be guarantee or fatal:
> 
> assert((failed_realloc_from_top != 0) == realloc_failures, "inconsistent state");
> 
> Add to next comment that inconsistent state of monitors are due to failed reallocation of synchronized objects (am I right?):
> 
> +     // Unlock all monitors here otherwise the interpreter will see a
> +     // mix of locked and unlocked monitors and be confused.
> 
> set_removed_monitors() is defined only under ASSERT:
> 
> +        array->element(i)->set_removed_monitors();
> 
> thread.hpp - fix comment:
> 
> +   // If a reallocation of scalar replaced objects fails during deoptimization,
> +   // we throw OOM and during exception propagation, pop the top
> +   // _frames_to_pop_failed_realloc interpreter frames corresponding
> +   // to the deoptimized compiler frame.
> 
> 
> vframe_hp.cpp
> 
> may be:
> 
> ((ObjectValue *)sv)->value()->is_null()
> 
> instead of
> 
> ((ObjectValue *)sv)->value()() == NULL
> 
> 
> TestDeoptOOM.java - @summary is wrong.
> Why you exclude OSR? You can exclude main() from compilation instead. And we need special OSR subtest to verify that frames are reconstructed correctly for OSR compilation. An other case is 2 scalarized allocations and one's field references an other.
> 
> Thanks,
> Vladimir
> 
> On 11/18/14 11:29 AM, Roland Westrelin wrote:
>> This is the OOM during deoptimization that causes a crash problem.
>> 
>> Here is the change I propose:
>> 
>> http://cr.openjdk.java.net/~roland/6898462/webrev.00/
>> 
>> I haven’t done a lot of testing. I’m looking for feedback.
>> 
>> With that change, the top interpreter frames that reference a failed reallocation are unconditionally popped as the exception is propagated. I record the number of frames to pop in the JavaThread structure and pop the frames in the interpreter runtime code that does the exception propagation.
>> 
>> Roland.
>>