RFR 8055008: Clean up code that saves the previous versions of redefined classes
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Aug 27 21:16:14 UTC 2014
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
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the serviceability-dev
mailing list