RFR 8055008: Clean up code that saves the previous versions of redefined classes
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Sep 3 18:20:44 UTC 2014
Thank you Serguei for these good review comments and questions. I
made the changes that you suggest and comments and you can see the
webrev here:
open webrev at http://cr.openjdk.java.net/~coleenp/8055008_04/
but comments are below:
On 9/3/14, 1:14 AM, serguei.spitsyn at oracle.com wrote:
> 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?
No, we don't remove methods individually.
> Is it because the method_data needs to be collected by the GC first?
No, we don't remove them because we remove all of the metadata together
with InstanceKlass being the holder for all the data. We have to do
method_data cleaning for all on_stack methods so that they don't refer
to not on_stack methods. The parenthetical comment was because we were
only doing this for emcp methods and now we are doing this for all
methods (it was a bug).
> If it is so, do we need a comment explaining it?
Ok, how about adding to the comment in the top of the methods() loop:
// At least one method is live in this previous version so clean
its MethodData.
// Reset dead EMCP methods not to get breakpoints.
// All methods are deallocated when all of the methods for this
class are no
// longer running.
Array<Method*>* method_refs = pv_node->methods();
if (method_refs != NULL) {
RC_TRACE(0x00000200, ("purge: previous methods length=%d",
method_refs->length()));
>
> 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'.
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 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.
>
> 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;
In this case, we have to add the previous_version because one of the old
methods in the previous klass is running. We keep the previous klass
on the list so that we can clean_weak_method_links in the obsolete methods.
But I changed it to below which is simpler:
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"));
} else {
the bit that loops around looking for emcp methods
}
add to previous version list.
> }
> // 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.
How about here (and reworded slightly)
// 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.
// We set the method->on_stack bit during safepoints for class
redefinition and
// class unloading and use this bit to set the is_running_emcp bit.
// After the safepoint, the on_stack bit is cleared and the
running emcp
// method may exit. If so, we would set a breakpoint in a
method that
// is never reached, but this won't be noticeable to the
programmer.
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));
}
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
>
More information about the hotspot-dev
mailing list