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

[Coleen Phillimore]

On 9/3/14, 4:22 PM, serguei.spitsyn at oracle.com wrote:
> Coleen,
>
>
> Thank you for the answers!
> They are helpful.
>
> The new webrev looks good to me.
> The only minor comment is about the fragment with long lines:
>
> 3633         RC_TRACE(0x00000400, ("add: EMCP method %s is on_stack " INTPTR_FORMAT, old_method->name_and_sig_as_C_string(), old_method));
> 3634       } else if (!old_method->is_obsolete()) {
> 3635         RC_TRACE(0x00000400, ("add: EMCP method %s is NOT on_stack " INTPTR_FORMAT, old_method->name_and_sig_as_C_string(), old_method));
> 3636       }
> Could you, please, split these lines?

Okay.  Thanks for the very thorough code review.
Coleen

>
>
> Thanks!
> Serguei
>
>
>
> On 9/3/14 12:55 PM, Coleen Phillimore wrote:
>>
>> Hi Serguei,
>>
>> I'm going to cut some things...
>>
>> <<cut>>
>>> Thank you for the explanation!
>>>
>>> There is also a potential scalability issue for class redefinitions 
>>> as we do a search through
>>> all these previous_versions and their old methods in the 
>>> mark_newly_obsolete_methods ().
>>> In the case of sub-sequential the same class redefinitions this 
>>> search will become worse and worse.
>>> However, I'm not suggesting to fix this now. :)
>>
>> I agree, it seems to take way too long to clear old methods once they 
>> are in the CodeCache.
>>>
>>>> It's different than just saying it's emcp.   It's emcp and it's 
>>>> running also so needs a breakpoint.
>>>>
>>>> The states are really:
>>>>
>>>>     is_obsolete() or !is_obsolete() same as is_emcp()
>>>>
>>>>     is_running_emcp() == !is_obsolete() && method->on_stack()
>>>>
>>>> We need to distinguish the running emcp methods from the 
>>>> non-running emcp methods.
>>>
>>> I suspect, sometimes this invariant is going to be broken:
>>>   is_running_emcp() == !is_obsolete() && method->on_stack()
>>>
>>> When the method has been finished and the on_stack is cleared,
>>> the method is_running_emcp bit can remain still uncleared, right?
>>> Would it be more simple just to use "!is_obsolete() && 
>>> method->on_stack()" ?
>>> It must be just in a couple of places.
>>
>> We only set on_stack when we do class redefinition and class 
>> unloading with MetadataOnStackMark.  After this safepoint, the bit is 
>> cleared.   We don't clear it when the method finishes.
>>
>> Is running_emcp is in only 4 places, but the place where we really 
>> need it (setting breakpoints) the "on_stack" bit isn't set because we 
>> don't do MetadataOnStackMark at that safepoint. It's sort of an 
>> expensive operation.
>>
>> So I need is_running_emcp() to capture the last known running state.
>>
>>>
>>>>
>>>> I guess we could just set breakpoints in all emcp methods whether 
>>>> they are running or not, and not have this flag. This seemed to 
>>>> preserve the old behavior better.
>>>
>>> I was thinking about the same but do not really have a preference.
>>> It is hard to estimate how big memory leak will cause these unneeded 
>>> breakpoints.
>>>
>>
>> It's not so much leakage, because the methods are there anyway but it 
>> seems inefficient to do breakpoints on methods that have exited.
>>
>> Setting these breakpoints looks expensive as well!
>>
>>> <<cut>>
>>> This is nice, thanks!
>>> I'm looking at the new webrev version now.
>>
>> Ok, let me know if there's anything else.
>> Coleen
>>
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 9/2/14 5:29 AM, Coleen Phillimore wrote:
>>>>>>
>>>>>> Serguei, I didn't answer one of your questions.
>>>>>>
>>>>>> On 8/28/14, 5:43 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> This bit is set during purging previous versions when all 
>>>>>>>> methods have been marked on_stack() if found in various 
>>>>>>>> places.  The bit is only used for setting breakpoints.
>>>>>>>
>>>>>>> I had to ask slightly different.
>>>>>>> "How precise must be the control of this bit?"
>>>>>>> Part of this question is the question below about what happens 
>>>>>>> when the method invocation is finished.
>>>>>>> I realized now that it can impact only setting breakpoints.
>>>>>>> Suppose, we did not clear the bit in time and then another 
>>>>>>> breakpoint is set.
>>>>>>> The only bad thing is that this new breakpoint will be useless.
>>>>>>
>>>>>> Yes.  We set the on_stack bit which causes setting the 
>>>>>> is_running_emcp bit during safepoints for class redefinition and 
>>>>>> class unloading.  After the safepoint, the on_stack bit is 
>>>>>> cleared.   After the safepoint, we may also set breakpoints using 
>>>>>> the is_running_emcp bit. If the method has exited we would set a 
>>>>>> breakpoint in a method that is never reached.  But this shouldn't 
>>>>>> be noticeable to the programmer.
>>>>>>
>>>>>> The method's is_running_emcp bit and maybe metadata would be 
>>>>>> cleaned up the next time we do class unloading at a safepoint.
>>>>>>
>>>>>>>
>>>>>>> But let me look at new webrev first to see if any update is 
>>>>>>> needed here.
>>>>>>>
>>>>>>
>>>>>> Yes, please review this again and let me know if this does what I 
>>>>>> claim it does.
>>>>>>
>>>>>> Thank you!
>>>>>> Coleen
>>>>>
>>>>
>>>
>>
>