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

[serguei.spitsyn at oracle.com]

Coleen,

It looks good in general.

But I have some questions and minor suggestions below.
||

src/share/vm/oops/instanceKlass.cpp

In the fragment below:

3488           if (!method->on_stack()) {
3489             if (method->is_running_emcp()) {
3490               method->set_running_emcp(false); // no breakpoints for non-running methods
3491             }
3492           } else {
3493             assert (method->is_obsolete() || method->is_running_emcp(),
3494                     "emcp method cannot run after emcp bit is cleared");
3495             // RC_TRACE macro has an embedded ResourceMark
3496             RC_TRACE(0x00000200,
3497               ("purge: %s(%s): prev method @%d in version @%d is alive",
3498               method->name()->as_C_string(),
3499               method->signature()->as_C_string(), j, version));
3500             if (method->method_data() != NULL) {
3501               // Clean out any weak method links for running methods
3502               // (also should include not EMCP methods)
3503               method->method_data()->clean_weak_method_links();
3504             }
3505           }


It is not clear to me what happens in the situation when the method is 
not running
(both emcp and obsolete cases). Why do we keep such methods, should we 
remove them?
Is it because the method_data needs to be collected by the GC first?
If it is so, do we need a comment explaining it?

Another question is about setting is_running_emcp.
Any method after redefinition can be in one of the three states:
    method->on_stack() ? 'is_obsolete' or 'is_emcp'
   !method->on_stack() ?  'to-be-removed'

I think, the 'is_running_emcp' flag is confusing (a suggestion: use 
'is_emcp').
It is not consistent with the 'is_obsolete' flag because we do not spell 
'is_running_obsolete'
as non-running obsolete methods hit the category 'to-be-removed'.

Just some thoughts. Sorry for looping around this. :(


A minor suggestion on the following fragment:

3611   if (cp_ref->on_stack()) {
3612     if (emcp_method_count == 0) {
3613       RC_TRACE(0x00000400, ("add: all methods are obsolete; no added EMCP refs"));
3614     } else {
3615       // At least one method is still running, check for EMCP methods
3616       for (int i = 0; i < old_methods->length(); i++) {
3617         Method* old_method = old_methods->at(i);
3618         if (!old_method->is_obsolete() && old_method->on_stack()) {
3619           // if EMCP method (not obsolete) is on the stack, mark as EMCP so that
3620           // we can add breakpoints for it.
3621           old_method->set_running_emcp(true);
3622           RC_TRACE(0x00000400, ("add: EMCP method %s is on_stack " INTPTR_FORMAT, old_method->name_and_sig_as_C_string(), old_method));
3623         } else if (!old_method->is_obsolete()) {
3624           RC_TRACE(0x00000400, ("add: EMCP method %s is NOT on_stack " INTPTR_FORMAT, old_method->name_and_sig_as_C_string(), old_method));
3625         }
3626       }
3627     }
3628
3629     RC_TRACE(0x00000400, ("add: scratch class added; one of its methods is on_stack"));
3630     assert(scratch_class->previous_versions() == NULL, "shouldn't have a previous version");
3631     scratch_class->link_previous_versions(previous_versions());
3632     link_previous_versions(scratch_class());
3633   } else {
3634     RC_TRACE(0x00000400, ("add: scratch class not added; no methods are running"));
3635   }

It'd be a simplification to reduce the indent like this:

   if (!cp_ref->on_stack()) {
     RC_TRACE(0x00000400, ("add: scratch class not added; no methods are running"));
     return;
   }
   if (emcp_method_count == 0) {
     RC_TRACE(0x00000400, ("add: all methods are obsolete; no added EMCP refs"));
     return;
   }
   // At least one method is still running, check for EMCP methods
   for (int i = 0; i < old_methods->length(); i++) {
     Method* old_method = old_methods->at(i);
     if (!old_method->is_obsolete() && old_method->on_stack()) {
       // if EMCP method (not obsolete) is on the stack, mark as EMCP so that
       // we can add breakpoints for it.
       old_method->set_running_emcp(true);\
       RC_TRACE(0x00000400, ("add: EMCP method %s is on_stack " INTPTR_FORMAT,
                            old_method->name_and_sig_as_C_string(), old_method));
     } else if (!old_method->is_obsolete()) {
       RC_TRACE(0x00000400, ("add: EMCP method %s is NOT on_stack " INTPTR_FORMAT,
                            old_method->name_and_sig_as_C_string(), old_method));
     }
   }
   RC_TRACE(0x00000400, ("add: scratch class added; one of its methods is on_stack"));
   assert(scratch_class->previous_versions() == NULL, "shouldn't have a previous version");
   scratch_class->link_previous_versions(previous_versions());
   link_previous_versions(scratch_class());


Also, from the 1st round review email exchange...
It'd be nice to add a comment somewhere above to explain this:

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


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