RFR(M): 8031752: Failed speculative optimization should be reattempted when root of compilation is different

[Christian Thalinger]

On Feb 21, 2014, at 12:31 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

> Hi Chris,
> 
> Thanks for looking at this.
> 
>> +     case dp->DataLayout::bit_data_tag:
>> 
>> This cannot be right.
> 
> Ok. But what part?

The dp-> part.

> 
>> 
>> +     // Move all cells of trap entry at dp left by "shift" cells
>> !     intptr_t* start = (intptr_t*)dp;
>> +     intptr_t* end = (intptr_t*)next_extra(dp);
>> +     for (intptr_t* ptr = start; ptr < end; ptr++) {
>> +       *(ptr-shift) = *ptr;
>>     }
>> 
>> Do you need to clear the shifted cells at the end?
> 
> It’s done when:
>    case DataLayout::no_tag:
>    case DataLayout::arg_info_data_tag:
> 
> The comment says the following cells need to be reset but it’s actually the previous “shift” cells. I fixed the comments in the webrev.

Got it.  Looks good.

> 
> Roland.
> 
>> 
>> On Feb 20, 2014, at 12:03 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>> 
>>> Good. Thank you, Roland.
>>> 
>>> Vladimir
>>> 
>>> On 2/20/14 11:53 AM, Roland Westrelin wrote:
>>>> Hi Vladimir,
>>>> 
>>>> Thanks for taking a look at this.
>>>> 
>>>>> I think it should be included into original changes.
>>>>> I would be nice if you add comments to this new code.
>>>>> As I understand the main fix is next lines:
>>>>> 
>>>>> +       if (!m->method_holder()->is_loader_alive(is_alive)) {
>>>>> +         shift += (int)((intptr_t*)next_extra(dp) - (intptr_t*)dp);
>>>>> 
>>>>> but without comments it is not clear what it does. Why 'shift' increment helps?
>>>> 
>>>> I updated the webrev in place with comments. Let me know if they help.
>>>> 
>>>> Roland.
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>> On 2/20/14 9:29 AM, Roland Westrelin wrote:
>>>>>> I found a bug in this already reviewed but not yet pushed change. The method that is referenced in the SpeculativeTrapData can be unloaded while the method that owns the SpeculativeTrapData remains live. This is possible with reflection (at least). Here is a bug fix for that with a test case. The webrev only includes the added change on top of what was already reviewed to make reviewing it easier. Or should I have it reviewed with another bug id?
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~roland/8031752/webrev.02-03/
>>>>>> 
>>>>>> Roland.
>>>>>> 
>>>> 
>> 
>