RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently

coleen.phillimore
Summary: Previous change was leaving scratch classes on CLD::_klasses
list which are reported to tracing

Tested with assert in tracing code and failed test, and mach4 tier1-5.

open webrev at http://cr.openjdk.java.net/~coleenp/8193622.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8193622

This change is relative to jdk/hs repository but will be moved to
jdk/jdk10 once the jdk/hs snapshot is integrated.

Thanks,
Coleen
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently

George Triantafillou
Hi Coleen,

Looks good.

-George

On 12/19/2017 6:42 AM, [hidden email] wrote:

> Summary: Previous change was leaving scratch classes on CLD::_klasses
> list which are reported to tracing
>
> Tested with assert in tracing code and failed test, and mach4 tier1-5.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8193622.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8193622
>
> This change is relative to jdk/hs repository but will be moved to
> jdk/jdk10 once the jdk/hs snapshot is integrated.
>
> Thanks,
> Coleen

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently

Daniel D. Daugherty
In reply to this post by coleen.phillimore
On 12/19/17 6:42 AM, [hidden email] wrote:
> Summary: Previous change was leaving scratch classes on CLD::_klasses
> list which are reported to tracing
>
> Tested with assert in tracing code and failed test, and mach4 tier1-5.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8193622.01/webrev

src/hotspot/share/classfile/classLoaderData.cpp
     No comments.

Thumbs up!

Dan



> bug link https://bugs.openjdk.java.net/browse/JDK-8193622
>
> This change is relative to jdk/hs repository but will be moved to
> jdk/jdk10 once the jdk/hs snapshot is integrated.
>
> Thanks,
> Coleen

Reply | Threaded
Open this post in threaded view
|

RE: RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently

Markus Gronlund
In reply to this post by coleen.phillimore
Hi Coleen,

Line 850 says: // Only constant pool entries have C heap memory to free.

But, with the change, there is an additional else clause that seems to process klasses.

Maybe remove the comment?

Also:

Is the metadata is_klass() check sufficient in allowing for a direct downcast to InstanceKlass* (not Klass*?). Maybe only InstanceKlass'es will ever be on the deallocation_list in this context?

In addition:

 857        // Remove the class so unloading events aren't triggered for
 858       // this class (scratch or error class) in do_unloading().
 859       remove_class(ik);

Does this not remove every klass? Not just a scratch or an error klass?

Thanks
Markus


-----Original Message-----
From: Coleen Phillimore
Sent: den 19 december 2017 12:43
To: [hidden email] runtime <[hidden email]>
Subject: RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently

Summary: Previous change was leaving scratch classes on CLD::_klasses list which are reported to tracing

Tested with assert in tracing code and failed test, and mach4 tier1-5.

open webrev at http://cr.openjdk.java.net/~coleenp/8193622.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8193622

This change is relative to jdk/hs repository but will be moved to
jdk/jdk10 once the jdk/hs snapshot is integrated.

Thanks,
Coleen
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently

coleen.phillimore


On 12/19/17 10:40 AM, Markus Gronlund wrote:
> Hi Coleen,
>
> Line 850 says: // Only constant pool entries have C heap memory to free.
>
> But, with the change, there is an additional else clause that seems to process klasses.
>
> Maybe remove the comment?

Yes, I meant to remove that comment.  Thanks.
>
> Also:
>
> Is the metadata is_klass() check sufficient in allowing for a direct downcast to InstanceKlass* (not Klass*?). Maybe only InstanceKlass'es will ever be on the deallocation_list in this context?

Yes, only InstanceKlass will be on the deallocate list.  I should change
this and free_deallocate_list with a future RFE.  I used is_klass()
because that's what free_deallocate_list had.

> In addition:
>
>   857        // Remove the class so unloading events aren't triggered for
>   858       // this class (scratch or error class) in do_unloading().
>   859       remove_class(ik);
>
> Does this not remove every klass? Not just a scratch or an error klass?

Only every Klass that is on the deallocate list, which are the scratch
and error klass.   The other classes aren't on this.

Thanks!
Coleen

> Thanks
> Markus
>
>
> -----Original Message-----
> From: Coleen Phillimore
> Sent: den 19 december 2017 12:43
> To: [hidden email] runtime <[hidden email]>
> Subject: RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently
>
> Summary: Previous change was leaving scratch classes on CLD::_klasses list which are reported to tracing
>
> Tested with assert in tracing code and failed test, and mach4 tier1-5.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8193622.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8193622
>
> This change is relative to jdk/hs repository but will be moved to
> jdk/jdk10 once the jdk/hs snapshot is integrated.
>
> Thanks,
> Coleen

Reply | Threaded
Open this post in threaded view
|

RE: RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently

Markus Gronlund
Thanks Coleen, looks good.

Markus

-----Original Message-----
From: Coleen Phillimore
Sent: den 19 december 2017 17:25
To: Markus Gronlund <[hidden email]>; [hidden email] runtime <[hidden email]>
Subject: Re: RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently



On 12/19/17 10:40 AM, Markus Gronlund wrote:
> Hi Coleen,
>
> Line 850 says: // Only constant pool entries have C heap memory to free.
>
> But, with the change, there is an additional else clause that seems to process klasses.
>
> Maybe remove the comment?

Yes, I meant to remove that comment.  Thanks.
>
> Also:
>
> Is the metadata is_klass() check sufficient in allowing for a direct downcast to InstanceKlass* (not Klass*?). Maybe only InstanceKlass'es will ever be on the deallocation_list in this context?

Yes, only InstanceKlass will be on the deallocate list.  I should change this and free_deallocate_list with a future RFE.  I used is_klass() because that's what free_deallocate_list had.

> In addition:
>
>   857        // Remove the class so unloading events aren't triggered for
>   858       // this class (scratch or error class) in do_unloading().
>   859       remove_class(ik);
>
> Does this not remove every klass? Not just a scratch or an error klass?

Only every Klass that is on the deallocate list, which are the scratch and error klass.   The other classes aren't on this.

Thanks!
Coleen

> Thanks
> Markus
>
>
> -----Original Message-----
> From: Coleen Phillimore
> Sent: den 19 december 2017 12:43
> To: [hidden email] runtime
> <[hidden email]>
> Subject: RFR (S) 8193622: JFR test TestUnloadingEventClass.java times
> out intermittently
>
> Summary: Previous change was leaving scratch classes on CLD::_klasses
> list which are reported to tracing
>
> Tested with assert in tracing code and failed test, and mach4 tier1-5.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8193622.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8193622
>
> This change is relative to jdk/hs repository but will be moved to
> jdk/jdk10 once the jdk/hs snapshot is integrated.
>
> Thanks,
> Coleen

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S) 8193622: JFR test TestUnloadingEventClass.java times out intermittently

coleen.phillimore
In reply to this post by Daniel D. Daugherty
Thanks Dan!
Coleen

On 12/19/17 10:39 AM, Daniel D. Daugherty wrote:

> On 12/19/17 6:42 AM, [hidden email] wrote:
>> Summary: Previous change was leaving scratch classes on CLD::_klasses
>> list which are reported to tracing
>>
>> Tested with assert in tracing code and failed test, and mach4 tier1-5.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8193622.01/webrev
>
> src/hotspot/share/classfile/classLoaderData.cpp
>     No comments.
>
> Thumbs up!
>
> Dan
>
>
>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8193622
>>
>> This change is relative to jdk/hs repository but will be moved to
>> jdk/jdk10 once the jdk/hs snapshot is integrated.
>>
>> Thanks,
>> Coleen
>