RFR(S): 8193933: Export ClassLoaderData claim state to support interleaved object traversal

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

RFR(S): 8193933: Export ClassLoaderData claim state to support interleaved object traversal

Markus Gronlund
Greetings,

 

Kindly asking for reviews for the following changes:

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8193933 

Webrev: http://cr.openjdk.java.net/~mgronlun/8193933/webrev01/ 

 

Additional information about the changes:

 

The only semantically relevant change here is moving the following member functions from private to public:

 

.         bool clear_claimed();

.         bool claimed() const;

.         bool claim();

 

In addition, I took the opportunity to include a few cleanups that have accumulated over time:

 

For GC folks, removing unused headers from ClassLoaderData.hpp revealed some .cpp's having missing includes for "memory/metaspaceCounters.hpp".

 

.         genCollectedHeap.cpp

.         parallelScavengeHeap.cpp

.         g1MonitoringSupport.cpp

.         universe.cpp

 

Each have now received an #include "memory/metaspaceCounters.hpp"

 

Summary changes to ClassLoaderData.hpp:

 

.         Removed unused header includes

.         Removed tracing components (moved to classLoaderData.cpp)

.         Removed unused member functions

.         Migrated private member functions to classLoaderData.cpp where possible

.         Reduced to a single private section and a single public section

.         Removed unused static variables ReadOnly and ReadWrite metaspaces

.         Removed/introduced a few forward declarations

.         Some updated member functions now wrapped in Debug/Non-product declarations

.         A very short attempt to group declared functionality (iterators, handles, statics)

 

Summary changes ClassLoaderData.cpp:

 

I realize it is pretty hopeless to view the changes to ClassLoaderData.cpp via the webrev.

Basically the changes attempts to reduce the amount of interleaving regarding the member function definitions for ClassLoaderDataGraph vs ClassLoaderData vs Iterators.

 

Here is the updated class layout:

 

ClassLoaderData::Dependendencies;

ClassLoaderData::ChunkedHandleList;

ClassLoaderData

ClassLoaderDataGraph

Iterators

 

In addition, the member function definition order reflects (although not completely) the groupings in ClassLoaderData.hpp.

 

Thank you

Markus

 

 

 

 
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8193933: Export ClassLoaderData claim state to support interleaved object traversal

coleen.phillimore

This bug is marked for JDK 10.  It is not appropriate to do this level
of cleanup for JDK 10.

I have only looked through the classLoaderData.hpp changes and I don't
agree with the gratuituous moving functions around.  I do like removing
header files and adding forward declarations, when possible, but that's
a smaller change.  Also a cleanup that should be done in JDK 11, not 10.

Thanks,
Coleen

On 12/21/17 9:14 AM, Markus Gronlund wrote:

> Greetings,
>
>  
>
> Kindly asking for reviews for the following changes:
>
>  
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8193933
>
> Webrev: http://cr.openjdk.java.net/~mgronlun/8193933/webrev01/
>
>  
>
> Additional information about the changes:
>
>  
>
> The only semantically relevant change here is moving the following member functions from private to public:
>
>  
>
> .         bool clear_claimed();
>
> .         bool claimed() const;
>
> .         bool claim();
>
>  
>
> In addition, I took the opportunity to include a few cleanups that have accumulated over time:
>
>  
>
> For GC folks, removing unused headers from ClassLoaderData.hpp revealed some .cpp's having missing includes for "memory/metaspaceCounters.hpp".
>
>  
>
> .         genCollectedHeap.cpp
>
> .         parallelScavengeHeap.cpp
>
> .         g1MonitoringSupport.cpp
>
> .         universe.cpp
>
>  
>
> Each have now received an #include "memory/metaspaceCounters.hpp"
>
>  
>
> Summary changes to ClassLoaderData.hpp:
>
>  
>
> .         Removed unused header includes
>
> .         Removed tracing components (moved to classLoaderData.cpp)
>
> .         Removed unused member functions
>
> .         Migrated private member functions to classLoaderData.cpp where possible
>
> .         Reduced to a single private section and a single public section
>
> .         Removed unused static variables ReadOnly and ReadWrite metaspaces
>
> .         Removed/introduced a few forward declarations
>
> .         Some updated member functions now wrapped in Debug/Non-product declarations
>
> .         A very short attempt to group declared functionality (iterators, handles, statics)
>
>  
>
> Summary changes ClassLoaderData.cpp:
>
>  
>
> I realize it is pretty hopeless to view the changes to ClassLoaderData.cpp via the webrev.
>
> Basically the changes attempts to reduce the amount of interleaving regarding the member function definitions for ClassLoaderDataGraph vs ClassLoaderData vs Iterators.
>
>  
>
> Here is the updated class layout:
>
>  
>
> ClassLoaderData::Dependendencies;
>
> ClassLoaderData::ChunkedHandleList;
>
> ClassLoaderData
>
> ClassLoaderDataGraph
>
> Iterators
>
>  
>
> In addition, the member function definition order reflects (although not completely) the groupings in ClassLoaderData.hpp.
>
>  
>
> Thank you
>
> Markus
>
>  
>
>  
>
>  
>
>  

Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8193933: Export ClassLoaderData claim state to support interleaved object traversal

Markus Gronlund
Hi Coleen,

We can postpone any cleanup activities in this area for 11.

Here is a minimal change:

http://cr.openjdk.java.net/~mgronlun/8193933/webrev02/

Thanks
Markus

-----Original Message-----
From: Coleen Phillimore
Sent: den 21 december 2017 17:15
To: [hidden email]
Subject: Re: RFR(S): 8193933: Export ClassLoaderData claim state to support interleaved object traversal


This bug is marked for JDK 10.  It is not appropriate to do this level of cleanup for JDK 10.

I have only looked through the classLoaderData.hpp changes and I don't agree with the gratuituous moving functions around.  I do like removing header files and adding forward declarations, when possible, but that's a smaller change.  Also a cleanup that should be done in JDK 11, not 10.

Thanks,
Coleen

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8193933: Export ClassLoaderData claim state to support interleaved object traversal

coleen.phillimore
Hi Markus,
This looks fine.  Happy new year!
Coleen

On 12/21/17 12:26 PM, Markus Gronlund wrote:

> Hi Coleen,
>
> We can postpone any cleanup activities in this area for 11.
>
> Here is a minimal change:
>
> http://cr.openjdk.java.net/~mgronlun/8193933/webrev02/
>
> Thanks
> Markus
>
> -----Original Message-----
> From: Coleen Phillimore
> Sent: den 21 december 2017 17:15
> To: [hidden email]
> Subject: Re: RFR(S): 8193933: Export ClassLoaderData claim state to support interleaved object traversal
>
>
> This bug is marked for JDK 10.  It is not appropriate to do this level of cleanup for JDK 10.
>
> I have only looked through the classLoaderData.hpp changes and I don't agree with the gratuituous moving functions around.  I do like removing header files and adding forward declarations, when possible, but that's a smaller change.  Also a cleanup that should be done in JDK 11, not 10.
>
> Thanks,
> Coleen
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8193933: Export ClassLoaderData claim state to support interleaved object traversal

harold seigel
+1

Harold


On 12/21/2017 1:10 PM, [hidden email] wrote:

> Hi Markus,
> This looks fine.  Happy new year!
> Coleen
>
> On 12/21/17 12:26 PM, Markus Gronlund wrote:
>> Hi Coleen,
>>
>> We can postpone any cleanup activities in this area for 11.
>>
>> Here is a minimal change:
>>
>> http://cr.openjdk.java.net/~mgronlun/8193933/webrev02/
>>
>> Thanks
>> Markus
>>
>> -----Original Message-----
>> From: Coleen Phillimore
>> Sent: den 21 december 2017 17:15
>> To: [hidden email]
>> Subject: Re: RFR(S): 8193933: Export ClassLoaderData claim state to
>> support interleaved object traversal
>>
>>
>> This bug is marked for JDK 10.  It is not appropriate to do this
>> level of cleanup for JDK 10.
>>
>> I have only looked through the classLoaderData.hpp changes and I
>> don't agree with the gratuituous moving functions around.  I do like
>> removing header files and adding forward declarations, when possible,
>> but that's a smaller change.  Also a cleanup that should be done in
>> JDK 11, not 10.
>>
>> Thanks,
>> Coleen
>>
>