RFR 8055008: Clean up code that saves the previous versions of redefined classes
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Sep 3 19:27:23 UTC 2014
Hi Coleen,
Please, see comments below.
On 9/3/14 11:20 AM, Coleen Phillimore wrote:
>
> 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/
Ok, thanks!
>
> 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).
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. :)
>
>> 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()));
The comment looks good.
Thanks!
>> 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 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.
>
> 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.
>
>>
>> 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.
You are right.
Sorry, I overlooked it.
>
> 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.
Agreed.
>> }
>> // 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));
> }
This is nice, thanks!
I'm looking at the new webrev version now.
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
>>
>
More information about the hotspot-dev
mailing list