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

[Coleen Phillimore]

On 8/27/14, 3:33 PM, Daniel D. Daugherty wrote:
> 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...

Do you mean methodHandle or MethodHandle above?  Or 
java_lang_reflect_Method?

The methodOop in little m methodHandles are not updated to refer to the 
new method, and Method* isn't either, so that really hasn't changed.

I'm not following the call sequence above.  But yes, I believe we could 
get into javaCalls::call() with an method, stop for a safepoint, and end 
up calling an obsolete method.   But that obsolete method is considered 
already running at that stage because the methodHandle logic marks it as 
such, so it's not considered a bug.

I ran the tests with the -XX:TraceRedefineClasses=0x2000 and didn't get 
the guarantee though, but that doesn't mean much.

I'm reading these mails in reverse...

Thanks,
Coleen

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