RFR 8055008: Clean up code that saves the previous versions of redefined classes

[Daniel D. Daugherty]

On 8/27/14 12:41 PM, serguei.spitsyn at oracle.com wrote:
> On 8/27/14 6:54 AM, Daniel D. Daugherty wrote:
>>
>> On 8/27/14 5:40 AM, serguei.spitsyn at oracle.com wrote:
>>> On 8/20/14 3:45 PM, Daniel D. Daugherty wrote:
>>>> On 8/20/14 2:01 PM, Coleen Phillimore wrote:
>>>>> On 8/20/14, 3:49 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>
>>>>>>> If an EMCP method is not running, should we save it on a 
>>>>>>> previous version list anyway so that we can make it obsolete if 
>>>>>>> it's redefined and made obsolete?
>>>>>>
>>>>>> I hope, Dan will catch me if I'm wrong...
>>>>>>
>>>>>> I think, we should not.
>>>>>> An EMCP method can not be made obsolete if it is not running.
>>>>>>
>>>>>
>>>>>
>>>>> It should be this way otherwise we'd have to hang onto things 
>>>>> forever.
>>>>
>>>> An EMCP method should only be made obsolete if a RedefineClasses() or
>>>> RetransformClasses() operation made it so. We should not be leveraging
>>>> off the obsolete-ness attribute to solve a life-cycle problem.
>>>>
>>>> In the pre-PGR world, we could trust GC to make a completely unused
>>>> EMCP method collectible and eventually our weak reference would go
>>>> away. Just because an EMCP method is not on a stack does not mean
>>>> that it is not used so we need a different way to determine whether
>>>> it is OK to no longer track an EMCP method.
>>>
>>> Most likely, you are right.
>>> But I'm not convinced yet. Sorry.
>>> A convincing point would be a test that shows this behavior.
>>> I understand that it is not an easy task to write such a test though.
>>> However, such a test would serve nicely if we want a different way
>>> to determine whether it is OK to no longer track an EMCP method.
>>
>> Running the Serviceability stack of tests with the following
>> -XX:TraceRedefineClasses=NN flags:
>>
>> //    0x00001000 |       4096 - detect calls to obsolete methods
>> //    0x00002000 |       8192 - fail a guarantee() in addition to 
>> detection
>>
>> will show failures of the guarantee(). I used to do this
>> periodically and then chase down the failures to make sure
>> only the "legitimate" races were happening. The reason that
>> we had to add these flags was to find all the places where
>> folks were caching methods in the VM. I think the last place
>> I found and fixed was in reflection.
>
> Ok, thanks.
> We threat this as buggy behavior, right?

Not necessarily. If it's because of a cached method that didn't
get updated to the new version, then that's a bug. However, if
it's because of a call that is in progress, then we have not
considered that a bug in the past.

I don't remember the exact details, but the dance is something
like this:

- jmethodID converted to methodHandle
- call to methodHandle prepared:
   - methodHandle -> methodOop
   - parameters marshalled
   - methodOop converted to method impl
- method called
   - somewhere in the middle of call sequence the
     method is redefined
   - jmethodID and methodHandle are updated to refer to the
     new method, but we already converted to the methodOop
     and the underlying method implementation for the final
     stages of the call
   - when we've actually called the now obsolete method,
     the guarantee() mentioned above fires and we have a
     false positive for the tracing code

Of course, now that we don't have methodOops, maybe this
doesn't happen anymore. I haven't done a test run with the
above flags enabled in quite some time...

Dan


>
> Thanks,
> Serguei
>
>>
>> Dan
>>
>>
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>>
>>>>>
>>>>>> BTW, I'm reviewing the webrev too, but probably it'd be better to 
>>>>>> switch to updated webrev after it is ready.
>>>>>
>>>>> Yes, this is a good idea.  I figured out why I made emcp methods 
>>>>> obsolete, and I'm fixing that as well as Dan's comments. Thanks!
>>>>
>>>> Cool! I'm looking forward to the next review.
>>>>
>>>> Dan
>>>>
>>>>
>>>>>
>>>>> Coleen
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>
>>>>
>>>
>>
>