Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-23 Thread serguei.spit...@oracle.com 

Fredrik,

The fix is good, good job!
It is still a good idea to run the nsk.jvmti and nsk.jdi tests to be safe.

Thanks,
Serguei

On 10/23/13 1:04 AM, Fredrik Arvidsson wrote:

Hi

The latest and hopefully last review version of this change can be 
found here:
http://cr.openjdk.java.net/~farvidsson/8024423/webrev.03/ 



Cheers
/F

On 2013-10-22 17:11, Stefan Karlsson wrote:

On 10/17/13 4:02 PM, Fredrik Arvidsson wrote:

Hi

I have added a new revision of my changes here: 
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.02/ 



I haven't looked at the patch i great detail, but I found this code:

+  void LoadedClassesClosure::do_klass(Klass* k) {
+// Collect all jclasses
+for (Klass* l = k; l != NULL; l = l->array_klass_or_null()) {
+  oop mirror = l->java_mirror();
+  _classStack.push((jclass) _env->jni_reference(mirror));
+}
+  }

I think you are adding the array klasses multiple times. 
ClassLoaderData::loaded_classes_do() is visiting all array klasses, 
so you shouldn't be traversing the l->array_klass_or_null() chain.


StefanK

The idea is to use a lock called metaspace_lock available inside the 
ClassLoaderData objects that the ClassLoaderDataGraph holds, this 
prevents classes to be added/removed/updated during the gathering 
phase. When iterating using the new LoadedClassesClosure 
implementation only array classes and instance classes that are 
marked as loaded will be visited. The LoadedClassesClosure 
implementation uses a Stack to store classes during the 
gathering phase. This changes the count-allocate-add-extract 
approach that was used before into a add-extract way of doing it 
instead.


I am still not sure how to do with the GetClassLoaderClasses to make 
it consistent. I would eventually like to get rid of the 
JvmtiGetLoadedClassesClosure and the use of the SystemDictionary if 
possible. But right now I just cant see how to get hold of the 
initiating loader for a class without using the SystemDictionary.


/Fredrik

On 2013-10-15 09:42, serguei.spit...@oracle.com wrote:


There are two questions on the list that we still need to resolve 
in this fix:
 (1) What is the best way to protect the work with CLDG from 
concurrent additions of CLD's to it
 (2) The *GetClassLoaderClasses* needs a fix as well to be 
consistent with the GetLoadedClasses


I had some private conversations with Fredrik and John Rose and
after some analysis came up with the suggestion:

  (1) Continue using the *SystemDictionary_lock* to protect 
consistency of the loaded classes data.
   The issue is that adding CLD's to the SLDG is not currently 
protected by the *SystemDictionary_lock*.
   I'm suggesting to add it to the 
*SystemDictionary::parse_stream* (please, see the webrev below).


  (2) There was some doubt that a similar fix for the 
*GetClassLoaderClasses* is needed because
   the CL+SD (taken from the host class) does not reference the 
associated anonymous classes.
   The question is: Can we consider the host's class CL as the 
initiating CL?
   My reading is that the answer to this question is: "Yes, we 
can".
   Even though the CL itself does not have references to its 
anonymous classes,
   there are references from the anonymous classes's CLD's to 
its CL.
   These references can be used in the *increment_with_loader* 
and *add_with_loader*

   the same way as it was before.

This is a webrev that includes the Fredrik's fix as a base plus the 
implemented suggestion:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/

Some help from the HotSpot team is needed to estimate if this 
approach is Ok and does not have rat holes in it.

Opinions are very welcome.

Thanks,
Serguei


On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested 
and here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to 
ClassLoaderDataGraph I have come to the conclusion that it should 
be safe to call this without any additional synchronization since 
newly loaded and added classes are appended to the end of its 
'list' of classes. This would mean that the call could 'miss' the 
classes added during the collection phase, but this is not a big 
issue. I hope that my conclusion is correct.


I believe that the 
JvmtiGetLoadedClasses::getClassLoaderClasses(...) method is to be 
left alone this time since I got the impression that only 
SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much 
of the code in this area, but I feel that it must wait until 
another time. The task to re-factor and

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-23 Thread Fredrik Arvidsson 

Hi

The latest and hopefully last review version of this change can be found 
here:
http://cr.openjdk.java.net/~farvidsson/8024423/webrev.03/ 



Cheers
/F

On 2013-10-22 17:11, Stefan Karlsson wrote:

On 10/17/13 4:02 PM, Fredrik Arvidsson wrote:

Hi

I have added a new revision of my changes here: 
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.02/ 



I haven't looked at the patch i great detail, but I found this code:

+  void LoadedClassesClosure::do_klass(Klass* k) {
+// Collect all jclasses
+for (Klass* l = k; l != NULL; l = l->array_klass_or_null()) {
+  oop mirror = l->java_mirror();
+  _classStack.push((jclass) _env->jni_reference(mirror));
+}
+  }

I think you are adding the array klasses multiple times. 
ClassLoaderData::loaded_classes_do() is visiting all array klasses, so 
you shouldn't be traversing the l->array_klass_or_null() chain.


StefanK

The idea is to use a lock called metaspace_lock available inside the 
ClassLoaderData objects that the ClassLoaderDataGraph holds, this 
prevents classes to be added/removed/updated during the gathering 
phase. When iterating using the new LoadedClassesClosure 
implementation only array classes and instance classes that are 
marked as loaded will be visited. The LoadedClassesClosure 
implementation uses a Stack to store classes during the 
gathering phase. This changes the count-allocate-add-extract approach 
that was used before into a add-extract way of doing it instead.


I am still not sure how to do with the GetClassLoaderClasses to make 
it consistent. I would eventually like to get rid of the 
JvmtiGetLoadedClassesClosure and the use of the SystemDictionary if 
possible. But right now I just cant see how to get hold of the 
initiating loader for a class without using the SystemDictionary.


/Fredrik

On 2013-10-15 09:42, serguei.spit...@oracle.com wrote:


There are two questions on the list that we still need to resolve in 
this fix:
 (1) What is the best way to protect the work with CLDG from 
concurrent additions of CLD's to it
 (2) The *GetClassLoaderClasses* needs a fix as well to be 
consistent with the GetLoadedClasses


I had some private conversations with Fredrik and John Rose and
after some analysis came up with the suggestion:

  (1) Continue using the *SystemDictionary_lock* to protect 
consistency of the loaded classes data.
   The issue is that adding CLD's to the SLDG is not currently 
protected by the *SystemDictionary_lock*.
   I'm suggesting to add it to the 
*SystemDictionary::parse_stream* (please, see the webrev below).


  (2) There was some doubt that a similar fix for the 
*GetClassLoaderClasses* is needed because
   the CL+SD (taken from the host class) does not reference the 
associated anonymous classes.
   The question is: Can we consider the host's class CL as the 
initiating CL?

   My reading is that the answer to this question is: "Yes, we can".
   Even though the CL itself does not have references to its 
anonymous classes,
   there are references from the anonymous classes's CLD's to 
its CL.
   These references can be used in the *increment_with_loader* 
and *add_with_loader*

   the same way as it was before.

This is a webrev that includes the Fredrik's fix as a base plus the 
implemented suggestion:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/

Some help from the HotSpot team is needed to estimate if this 
approach is Ok and does not have rat holes in it.

Opinions are very welcome.

Thanks,
Serguei


On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested 
and here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to 
ClassLoaderDataGraph I have come to the conclusion that it should 
be safe to call this without any additional synchronization since 
newly loaded and added classes are appended to the end of its 
'list' of classes. This would mean that the call could 'miss' the 
classes added during the collection phase, but this is not a big 
issue. I hope that my conclusion is correct.


I believe that the 
JvmtiGetLoadedClasses::getClassLoaderClasses(...) method is to be 
left alone this time since I got the impression that only 
SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much 
of the code in this area, but I feel that it must wait until 
another time. The task to re-factor and simplify would quickly grow 
out of hands I'm afraid :)


Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spit...@oracle.com wrote:

Hi Frederik,


Thank you for jumping on this i

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-22 Thread serguei.spit...@oracle.com 

Hi Fredrik,

The fix looks good modulo the comment from Stefan about the array classes.
Thank you for addressing the comments from the 1st round!
Please, file an RFE on the GetClassLoaderClasses to keep track of it.

Thanks,
Serguei


On 10/17/13 7:02 AM, Fredrik Arvidsson wrote:

Hi

I have added a new revision of my changes here: 
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.02/ 



The idea is to use a lock called metaspace_lock available inside the 
ClassLoaderData objects that the ClassLoaderDataGraph holds, this 
prevents classes to be added/removed/updated during the gathering 
phase. When iterating using the new LoadedClassesClosure 
implementation only array classes and instance classes that are marked 
as loaded will be visited. The LoadedClassesClosure implementation 
uses a Stack to store classes during the gathering phase. This 
changes the count-allocate-add-extract approach that was used before 
into a add-extract way of doing it instead.


I am still not sure how to do with the GetClassLoaderClasses to make 
it consistent. I would eventually like to get rid of the 
JvmtiGetLoadedClassesClosure and the use of the SystemDictionary if 
possible. But right now I just cant see how to get hold of the 
initiating loader for a class without using the SystemDictionary.


/Fredrik

On 2013-10-15 09:42, serguei.spit...@oracle.com wrote:


There are two questions on the list that we still need to resolve in 
this fix:
 (1) What is the best way to protect the work with CLDG from 
concurrent additions of CLD's to it
 (2) The *GetClassLoaderClasses* needs a fix as well to be consistent 
with the GetLoadedClasses


I had some private conversations with Fredrik and John Rose and
after some analysis came up with the suggestion:

  (1) Continue using the *SystemDictionary_lock* to protect 
consistency of the loaded classes data.
   The issue is that adding CLD's to the SLDG is not currently 
protected by the *SystemDictionary_lock*.
   I'm suggesting to add it to the 
*SystemDictionary::parse_stream* (please, see the webrev below).


  (2) There was some doubt that a similar fix for the 
*GetClassLoaderClasses* is needed because
   the CL+SD (taken from the host class) does not reference the 
associated anonymous classes.
   The question is: Can we consider the host's class CL as the 
initiating CL?

   My reading is that the answer to this question is: "Yes, we can".
   Even though the CL itself does not have references to its 
anonymous classes,

   there are references from the anonymous classes's CLD's to its CL.
   These references can be used in the *increment_with_loader* 
and *add_with_loader*

   the same way as it was before.

This is a webrev that includes the Fredrik's fix as a base plus the 
implemented suggestion:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/

Some help from the HotSpot team is needed to estimate if this 
approach is Ok and does not have rat holes in it.

Opinions are very welcome.

Thanks,
Serguei


On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested and 
here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to ClassLoaderDataGraph 
I have come to the conclusion that it should be safe to call this 
without any additional synchronization since newly loaded and added 
classes are appended to the end of its 'list' of classes. This would 
mean that the call could 'miss' the classes added during the 
collection phase, but this is not a big issue. I hope that my 
conclusion is correct.


I believe that the JvmtiGetLoadedClasses::getClassLoaderClasses(...) 
method is to be left alone this time since I got the impression that 
only SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much 
of the code in this area, but I feel that it must wait until another 
time. The task to re-factor and simplify would quickly grow out of 
hands I'm afraid :)


Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spit...@oracle.com wrote:

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*
2333   int src_index = 0;
2334   while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340 int hash_index = 0;
2341 while (hash_index < hash_len) {
2342   dest[dest_index++] = hash_buf[hash_index++];
2343 }
2344   }

The above can be simplified a little bit:
// Add the actual class name
for (int src_index = 0; src_index < src_length; ) {
  dest[dest_index++] = src[src_index++];

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-22 Thread Stefan Karlsson 

On 10/17/13 4:02 PM, Fredrik Arvidsson wrote:

Hi

I have added a new revision of my changes here: 
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.02/ 



I haven't looked at the patch i great detail, but I found this code:

+  void LoadedClassesClosure::do_klass(Klass* k) {
+// Collect all jclasses
+for (Klass* l = k; l != NULL; l = l->array_klass_or_null()) {
+  oop mirror = l->java_mirror();
+  _classStack.push((jclass) _env->jni_reference(mirror));
+}
+  }

I think you are adding the array klasses multiple times. 
ClassLoaderData::loaded_classes_do() is visiting all array klasses, so 
you shouldn't be traversing the l->array_klass_or_null() chain.


StefanK

The idea is to use a lock called metaspace_lock available inside the 
ClassLoaderData objects that the ClassLoaderDataGraph holds, this 
prevents classes to be added/removed/updated during the gathering 
phase. When iterating using the new LoadedClassesClosure 
implementation only array classes and instance classes that are marked 
as loaded will be visited. The LoadedClassesClosure implementation 
uses a Stack to store classes during the gathering phase. This 
changes the count-allocate-add-extract approach that was used before 
into a add-extract way of doing it instead.


I am still not sure how to do with the GetClassLoaderClasses to make 
it consistent. I would eventually like to get rid of the 
JvmtiGetLoadedClassesClosure and the use of the SystemDictionary if 
possible. But right now I just cant see how to get hold of the 
initiating loader for a class without using the SystemDictionary.


/Fredrik

On 2013-10-15 09:42, serguei.spit...@oracle.com wrote:


There are two questions on the list that we still need to resolve in 
this fix:
 (1) What is the best way to protect the work with CLDG from 
concurrent additions of CLD's to it
 (2) The *GetClassLoaderClasses* needs a fix as well to be consistent 
with the GetLoadedClasses


I had some private conversations with Fredrik and John Rose and
after some analysis came up with the suggestion:

  (1) Continue using the *SystemDictionary_lock* to protect 
consistency of the loaded classes data.
   The issue is that adding CLD's to the SLDG is not currently 
protected by the *SystemDictionary_lock*.
   I'm suggesting to add it to the 
*SystemDictionary::parse_stream* (please, see the webrev below).


  (2) There was some doubt that a similar fix for the 
*GetClassLoaderClasses* is needed because
   the CL+SD (taken from the host class) does not reference the 
associated anonymous classes.
   The question is: Can we consider the host's class CL as the 
initiating CL?

   My reading is that the answer to this question is: "Yes, we can".
   Even though the CL itself does not have references to its 
anonymous classes,

   there are references from the anonymous classes's CLD's to its CL.
   These references can be used in the *increment_with_loader* 
and *add_with_loader*

   the same way as it was before.

This is a webrev that includes the Fredrik's fix as a base plus the 
implemented suggestion:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/

Some help from the HotSpot team is needed to estimate if this 
approach is Ok and does not have rat holes in it.

Opinions are very welcome.

Thanks,
Serguei


On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested and 
here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to ClassLoaderDataGraph 
I have come to the conclusion that it should be safe to call this 
without any additional synchronization since newly loaded and added 
classes are appended to the end of its 'list' of classes. This would 
mean that the call could 'miss' the classes added during the 
collection phase, but this is not a big issue. I hope that my 
conclusion is correct.


I believe that the JvmtiGetLoadedClasses::getClassLoaderClasses(...) 
method is to be left alone this time since I got the impression that 
only SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much 
of the code in this area, but I feel that it must wait until another 
time. The task to re-factor and simplify would quickly grow out of 
hands I'm afraid :)


Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spit...@oracle.com wrote:

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*
2333   int src_index = 0;
2334   while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340 int hash_index =

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-22 Thread Coleen Phillimore 


Frederik,

This code looks good and addresses my concerns.  It's an improvement and 
resolves this issue. I suggest that we should wait to see what 
initiating loader means for anonymous classes since the jvm doesn't have 
that information readily before tackling that.  But ship this fix 
definitely.


thanks,
Coleen

On 10/17/2013 10:02 AM, Fredrik Arvidsson wrote:

Hi

I have added a new revision of my changes here: 
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.02/ 



The idea is to use a lock called metaspace_lock available inside the 
ClassLoaderData objects that the ClassLoaderDataGraph holds, this 
prevents classes to be added/removed/updated during the gathering 
phase. When iterating using the new LoadedClassesClosure 
implementation only array classes and instance classes that are marked 
as loaded will be visited. The LoadedClassesClosure implementation 
uses a Stack to store classes during the gathering phase. This 
changes the count-allocate-add-extract approach that was used before 
into a add-extract way of doing it instead.


I am still not sure how to do with the GetClassLoaderClasses to make 
it consistent. I would eventually like to get rid of the 
JvmtiGetLoadedClassesClosure and the use of the SystemDictionary if 
possible. But right now I just cant see how to get hold of the 
initiating loader for a class without using the SystemDictionary.


/Fredrik

On 2013-10-15 09:42, serguei.spit...@oracle.com wrote:


There are two questions on the list that we still need to resolve in 
this fix:
 (1) What is the best way to protect the work with CLDG from 
concurrent additions of CLD's to it
 (2) The *GetClassLoaderClasses* needs a fix as well to be consistent 
with the GetLoadedClasses


I had some private conversations with Fredrik and John Rose and
after some analysis came up with the suggestion:

  (1) Continue using the *SystemDictionary_lock* to protect 
consistency of the loaded classes data.
   The issue is that adding CLD's to the SLDG is not currently 
protected by the *SystemDictionary_lock*.
   I'm suggesting to add it to the 
*SystemDictionary::parse_stream* (please, see the webrev below).


  (2) There was some doubt that a similar fix for the 
*GetClassLoaderClasses* is needed because
   the CL+SD (taken from the host class) does not reference the 
associated anonymous classes.
   The question is: Can we consider the host's class CL as the 
initiating CL?

   My reading is that the answer to this question is: "Yes, we can".
   Even though the CL itself does not have references to its 
anonymous classes,

   there are references from the anonymous classes's CLD's to its CL.
   These references can be used in the *increment_with_loader* 
and *add_with_loader*

   the same way as it was before.

This is a webrev that includes the Fredrik's fix as a base plus the 
implemented suggestion:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/

Some help from the HotSpot team is needed to estimate if this 
approach is Ok and does not have rat holes in it.

Opinions are very welcome.

Thanks,
Serguei


On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested and 
here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to ClassLoaderDataGraph 
I have come to the conclusion that it should be safe to call this 
without any additional synchronization since newly loaded and added 
classes are appended to the end of its 'list' of classes. This would 
mean that the call could 'miss' the classes added during the 
collection phase, but this is not a big issue. I hope that my 
conclusion is correct.


I believe that the JvmtiGetLoadedClasses::getClassLoaderClasses(...) 
method is to be left alone this time since I got the impression that 
only SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much 
of the code in this area, but I feel that it must wait until another 
time. The task to re-factor and simplify would quickly grow out of 
hands I'm afraid :)


Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spit...@oracle.com wrote:

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*
2333   int src_index = 0;
2334   while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340 int hash_index = 0;
2341 while (hash_index < hash_len) {
2342   dest[dest_index++] = hash_buf[hash_index++];
2343 }
2344   }

The above can be simplified a little bit:
// Add the actual class name
for (int sr

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-18 Thread serguei.spit...@oracle.com 

Hi Fredrik,

I like this approach in general but am still looking into some details.

There is also one concern from Coleen about the anonymous classes:
they appear in the CLDG/CLD partially initialized.

For details, look at the function SystemDictionary::parse_stream()
that is called from the Unsafe_DefineAnonymousClass_impl().

Thanks,
Serguei



On 10/17/13 7:02 AM, Fredrik Arvidsson wrote:

Hi

I have added a new revision of my changes here: 
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.02/ 



The idea is to use a lock called metaspace_lock available inside the 
ClassLoaderData objects that the ClassLoaderDataGraph holds, this 
prevents classes to be added/removed/updated during the gathering 
phase. When iterating using the new LoadedClassesClosure 
implementation only array classes and instance classes that are marked 
as loaded will be visited. The LoadedClassesClosure implementation 
uses a Stack to store classes during the gathering phase. This 
changes the count-allocate-add-extract approach that was used before 
into a add-extract way of doing it instead.


I am still not sure how to do with the GetClassLoaderClasses to make 
it consistent. I would eventually like to get rid of the 
JvmtiGetLoadedClassesClosure and the use of the SystemDictionary if 
possible. But right now I just cant see how to get hold of the 
initiating loader for a class without using the SystemDictionary.


/Fredrik

On 2013-10-15 09:42, serguei.spit...@oracle.com wrote:


There are two questions on the list that we still need to resolve in 
this fix:
 (1) What is the best way to protect the work with CLDG from 
concurrent additions of CLD's to it
 (2) The *GetClassLoaderClasses* needs a fix as well to be consistent 
with the GetLoadedClasses


I had some private conversations with Fredrik and John Rose and
after some analysis came up with the suggestion:

  (1) Continue using the *SystemDictionary_lock* to protect 
consistency of the loaded classes data.
   The issue is that adding CLD's to the SLDG is not currently 
protected by the *SystemDictionary_lock*.
   I'm suggesting to add it to the 
*SystemDictionary::parse_stream* (please, see the webrev below).


  (2) There was some doubt that a similar fix for the 
*GetClassLoaderClasses* is needed because
   the CL+SD (taken from the host class) does not reference the 
associated anonymous classes.
   The question is: Can we consider the host's class CL as the 
initiating CL?

   My reading is that the answer to this question is: "Yes, we can".
   Even though the CL itself does not have references to its 
anonymous classes,

   there are references from the anonymous classes's CLD's to its CL.
   These references can be used in the *increment_with_loader* 
and *add_with_loader*

   the same way as it was before.

This is a webrev that includes the Fredrik's fix as a base plus the 
implemented suggestion:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/

Some help from the HotSpot team is needed to estimate if this 
approach is Ok and does not have rat holes in it.

Opinions are very welcome.

Thanks,
Serguei


On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested and 
here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to ClassLoaderDataGraph 
I have come to the conclusion that it should be safe to call this 
without any additional synchronization since newly loaded and added 
classes are appended to the end of its 'list' of classes. This would 
mean that the call could 'miss' the classes added during the 
collection phase, but this is not a big issue. I hope that my 
conclusion is correct.


I believe that the JvmtiGetLoadedClasses::getClassLoaderClasses(...) 
method is to be left alone this time since I got the impression that 
only SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much 
of the code in this area, but I feel that it must wait until another 
time. The task to re-factor and simplify would quickly grow out of 
hands I'm afraid :)


Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spit...@oracle.com wrote:

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*
2333   int src_index = 0;
2334   while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340 int hash_index = 0;
2341 while (hash_index < hash_len) {
2342   dest[dest_index++] = hash_buf[hash_index++];
2343 }
2344   }

The above can be simplified a little bit:
// Add the actua

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-17 Thread Fredrik Arvidsson 

Hi

I have added a new revision of my changes here: 
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.02/ 



The idea is to use a lock called metaspace_lock available inside the 
ClassLoaderData objects that the ClassLoaderDataGraph holds, this 
prevents classes to be added/removed/updated during the gathering phase. 
When iterating using the new LoadedClassesClosure implementation only 
array classes and instance classes that are marked as loaded will be 
visited. The LoadedClassesClosure implementation uses a Stack to 
store classes during the gathering phase. This changes the 
count-allocate-add-extract approach that was used before into a 
add-extract way of doing it instead.


I am still not sure how to do with the GetClassLoaderClasses to make it 
consistent. I would eventually like to get rid of the 
JvmtiGetLoadedClassesClosure and the use of the SystemDictionary if 
possible. But right now I just cant see how to get hold of the 
initiating loader for a class without using the SystemDictionary.


/Fredrik

On 2013-10-15 09:42, serguei.spit...@oracle.com wrote:


There are two questions on the list that we still need to resolve in 
this fix:
 (1) What is the best way to protect the work with CLDG from 
concurrent additions of CLD's to it
 (2) The *GetClassLoaderClasses* needs a fix as well to be consistent 
with the GetLoadedClasses


I had some private conversations with Fredrik and John Rose and
after some analysis came up with the suggestion:

  (1) Continue using the *SystemDictionary_lock* to protect 
consistency of the loaded classes data.
   The issue is that adding CLD's to the SLDG is not currently 
protected by the *SystemDictionary_lock*.
   I'm suggesting to add it to the 
*SystemDictionary::parse_stream* (please, see the webrev below).


  (2) There was some doubt that a similar fix for the 
*GetClassLoaderClasses* is needed because
   the CL+SD (taken from the host class) does not reference the 
associated anonymous classes.
   The question is: Can we consider the host's class CL as the 
initiating CL?

   My reading is that the answer to this question is: "Yes, we can".
   Even though the CL itself does not have references to its 
anonymous classes,

   there are references from the anonymous classes's CLD's to its CL.
   These references can be used in the *increment_with_loader* and 
*add_with_loader*

   the same way as it was before.

This is a webrev that includes the Fredrik's fix as a base plus the 
implemented suggestion:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/

Some help from the HotSpot team is needed to estimate if this approach 
is Ok and does not have rat holes in it.

Opinions are very welcome.

Thanks,
Serguei


On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested and 
here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to ClassLoaderDataGraph 
I have come to the conclusion that it should be safe to call this 
without any additional synchronization since newly loaded and added 
classes are appended to the end of its 'list' of classes. This would 
mean that the call could 'miss' the classes added during the 
collection phase, but this is not a big issue. I hope that my 
conclusion is correct.


I believe that the JvmtiGetLoadedClasses::getClassLoaderClasses(...) 
method is to be left alone this time since I got the impression that 
only SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much of 
the code in this area, but I feel that it must wait until another 
time. The task to re-factor and simplify would quickly grow out of 
hands I'm afraid :)


Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spit...@oracle.com wrote:

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*
2333   int src_index = 0;
2334   while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340 int hash_index = 0;
2341 while (hash_index < hash_len) {
2342   dest[dest_index++] = hash_buf[hash_index++];
2343 }
2344   }

The above can be simplified a little bit:
// Add the actual class name
for (int src_index = 0; src_index < src_length; ) {
  dest[dest_index++] = src[src_index++];
}
// If we have a hash, append it
for (int hash_index = 0; hash_index < hash_len; ) {
  dest[dest_index++] = hash_buf[hash_index++];
}

The conditionif(hash_len > 0)  is covered by the loop itself.

Style-related comments:
  -wrong indent at the lines: 2316-2319

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-15 Thread Fredrik Arvidsson 

I think this looks great.

/Fredrik

On 2013-10-15 09:42, serguei.spit...@oracle.com wrote:


There are two questions on the list that we still need to resolve in 
this fix:
 (1) What is the best way to protect the work with CLDG from 
concurrent additions of CLD's to it
 (2) The *GetClassLoaderClasses* needs a fix as well to be consistent 
with the GetLoadedClasses


I had some private conversations with Fredrik and John Rose and
after some analysis came up with the suggestion:

  (1) Continue using the *SystemDictionary_lock* to protect 
consistency of the loaded classes data.
   The issue is that adding CLD's to the SLDG is not currently 
protected by the *SystemDictionary_lock*.
   I'm suggesting to add it to the 
*SystemDictionary::parse_stream* (please, see the webrev below).


  (2) There was some doubt that a similar fix for the 
*GetClassLoaderClasses* is needed because
   the CL+SD (taken from the host class) does not reference the 
associated anonymous classes.
   The question is: Can we consider the host's class CL as the 
initiating CL?

   My reading is that the answer to this question is: "Yes, we can".
   Even though the CL itself does not have references to its 
anonymous classes,

   there are references from the anonymous classes's CLD's to its CL.
   These references can be used in the *increment_with_loader* and 
*add_with_loader*

   the same way as it was before.

This is a webrev that includes the Fredrik's fix as a base plus the 
implemented suggestion:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/

Some help from the HotSpot team is needed to estimate if this approach 
is Ok and does not have rat holes in it.

Opinions are very welcome.

Thanks,
Serguei


On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested and 
here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to ClassLoaderDataGraph 
I have come to the conclusion that it should be safe to call this 
without any additional synchronization since newly loaded and added 
classes are appended to the end of its 'list' of classes. This would 
mean that the call could 'miss' the classes added during the 
collection phase, but this is not a big issue. I hope that my 
conclusion is correct.


I believe that the JvmtiGetLoadedClasses::getClassLoaderClasses(...) 
method is to be left alone this time since I got the impression that 
only SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much of 
the code in this area, but I feel that it must wait until another 
time. The task to re-factor and simplify would quickly grow out of 
hands I'm afraid :)


Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spit...@oracle.com wrote:

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*
2333   int src_index = 0;
2334   while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340 int hash_index = 0;
2341 while (hash_index < hash_len) {
2342   dest[dest_index++] = hash_buf[hash_index++];
2343 }
2344   }

The above can be simplified a little bit:
// Add the actual class name
for (int src_index = 0; src_index < src_length; ) {
  dest[dest_index++] = src[src_index++];
}
// If we have a hash, append it
for (int hash_index = 0; hash_index < hash_len; ) {
  dest[dest_index++] = hash_buf[hash_index++];
}

The conditionif(hash_len > 0)  is covered by the loop itself.

Style-related comments:
  -wrong indent at the lines: 2316-2319
  - need a space after the 'if' at the lines: 2315, 2339


*src/share/vm/prims/jvmtiGetLoadedClasses.cpp

*The copyright comment must be up-to-date.
The comments at the lines 35-38, 258-260 are not valid anymore.


> In this review I still have an outstanding unanswered question about
> the need to synchronize the access to the:
 
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);

> and
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);

This kind of walking is usually done at safepoint in a VM_Operation.
You will find many examples in the jvmtiEnv.cpp, for instance, 
VM_GetAllStackTraces.
The suggestion from Mikael is good too.

Should this fix include the getClassLoaderClasses() as well?
I'm still trying to realize the impact and consistency of this fix. :(

What tests are you going to run for verification?

Thanks,
Serguei
*
*On 9/30/13 4:45 AM, Fredrik Arvidsson wrote:

Hi

Please help me to review the changes below:

Jira case: https://bugs.openjdk.java.net/browse/JDK-8024423
Webrev: 
http://cr.

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-15 Thread serguei.spit...@oracle.com 


There are two questions on the list that we still need to resolve in 
this fix:
 (1) What is the best way to protect the work with CLDG from concurrent 
additions of CLD's to it
 (2) The *GetClassLoaderClasses* needs a fix as well to be consistent 
with the GetLoadedClasses


I had some private conversations with Fredrik and John Rose and
after some analysis came up with the suggestion:

  (1) Continue using the *SystemDictionary_lock* to protect consistency 
of the loaded classes data.
   The issue is that adding CLD's to the SLDG is not currently 
protected by the *SystemDictionary_lock*.
   I'm suggesting to add it to the *SystemDictionary::parse_stream* 
(please, see the webrev below).


  (2) There was some doubt that a similar fix for the 
*GetClassLoaderClasses* is needed because
   the CL+SD (taken from the host class) does not reference the 
associated anonymous classes.
   The question is: Can we consider the host's class CL as the 
initiating CL?

   My reading is that the answer to this question is: "Yes, we can".
   Even though the CL itself does not have references to its 
anonymous classes,

   there are references from the anonymous classes's CLD's to its CL.
   These references can be used in the *increment_with_loader* and 
*add_with_loader*

   the same way as it was before.

This is a webrev that includes the Fredrik's fix as a base plus the 
implemented suggestion:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/

Some help from the HotSpot team is needed to estimate if this approach 
is Ok and does not have rat holes in it.

Opinions are very welcome.

Thanks,
Serguei


On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested and 
here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to ClassLoaderDataGraph I 
have come to the conclusion that it should be safe to call this 
without any additional synchronization since newly loaded and added 
classes are appended to the end of its 'list' of classes. This would 
mean that the call could 'miss' the classes added during the 
collection phase, but this is not a big issue. I hope that my 
conclusion is correct.


I believe that the JvmtiGetLoadedClasses::getClassLoaderClasses(...) 
method is to be left alone this time since I got the impression that 
only SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much of 
the code in this area, but I feel that it must wait until another 
time. The task to re-factor and simplify would quickly grow out of 
hands I'm afraid :)


Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spit...@oracle.com wrote:

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*
2333   int src_index = 0;
2334   while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340 int hash_index = 0;
2341 while (hash_index < hash_len) {
2342   dest[dest_index++] = hash_buf[hash_index++];
2343 }
2344   }

The above can be simplified a little bit:
// Add the actual class name
for (int src_index = 0; src_index < src_length; ) {
  dest[dest_index++] = src[src_index++];
}
// If we have a hash, append it
for (int hash_index = 0; hash_index < hash_len; ) {
  dest[dest_index++] = hash_buf[hash_index++];
}

The conditionif(hash_len > 0)  is covered by the loop itself.

Style-related comments:
  -wrong indent at the lines: 2316-2319
  - need a space after the 'if' at the lines: 2315, 2339


*src/share/vm/prims/jvmtiGetLoadedClasses.cpp

*The copyright comment must be up-to-date.
The comments at the lines 35-38, 258-260 are not valid anymore.


> In this review I still have an outstanding unanswered question about
> the need to synchronize the access to the:
 
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);

> and
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);

This kind of walking is usually done at safepoint in a VM_Operation.
You will find many examples in the jvmtiEnv.cpp, for instance, 
VM_GetAllStackTraces.
The suggestion from Mikael is good too.

Should this fix include the getClassLoaderClasses() as well?
I'm still trying to realize the impact and consistency of this fix. :(

What tests are you going to run for verification?

Thanks,
Serguei
*
*On 9/30/13 4:45 AM, Fredrik Arvidsson wrote:

Hi

Please help me to review the changes below:

Jira case: https://bugs.openjdk.java.net/browse/JDK-8024423
Webrev: 
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-03 Thread serguei.spit...@oracle.com 

Hi Frederik,

Will review this tomorrow.
Sorry for the latency, was busy with other things.

In general, I do not like the idea to skip the synchronization.
Also, it is important to keep the information from JVMTI consistent.
The data returned by the getLoadedClasses and getClassLoaderClasses 
should match.
We also need to make sure it is consistent with other info the JVMTI 
provides.

This requirement is not for improvement or simplification.

But more tomorrow...

Thanks,
Serguei


On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested and 
here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to ClassLoaderDataGraph I 
have come to the conclusion that it should be safe to call this 
without any additional synchronization since newly loaded and added 
classes are appended to the end of its 'list' of classes. This would 
mean that the call could 'miss' the classes added during the 
collection phase, but this is not a big issue. I hope that my 
conclusion is correct.


I believe that the JvmtiGetLoadedClasses::getClassLoaderClasses(...) 
method is to be left alone this time since I got the impression that 
only SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much of 
the code in this area, but I feel that it must wait until another 
time. The task to re-factor and simplify would quickly grow out of 
hands I'm afraid :)


Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spit...@oracle.com wrote:

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*
2333   int src_index = 0;
2334   while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340 int hash_index = 0;
2341 while (hash_index < hash_len) {
2342   dest[dest_index++] = hash_buf[hash_index++];
2343 }
2344   }

The above can be simplified a little bit:
// Add the actual class name
for (int src_index = 0; src_index < src_length; ) {
  dest[dest_index++] = src[src_index++];
}
// If we have a hash, append it
for (int hash_index = 0; hash_index < hash_len; ) {
  dest[dest_index++] = hash_buf[hash_index++];
}

The conditionif(hash_len > 0)  is covered by the loop itself.

Style-related comments:
  -wrong indent at the lines: 2316-2319
  - need a space after the 'if' at the lines: 2315, 2339


*src/share/vm/prims/jvmtiGetLoadedClasses.cpp

*The copyright comment must be up-to-date.
The comments at the lines 35-38, 258-260 are not valid anymore.


> In this review I still have an outstanding unanswered question about
> the need to synchronize the access to the:
 
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);

> and
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);

This kind of walking is usually done at safepoint in a VM_Operation.
You will find many examples in the jvmtiEnv.cpp, for instance, 
VM_GetAllStackTraces.
The suggestion from Mikael is good too.

Should this fix include the getClassLoaderClasses() as well?
I'm still trying to realize the impact and consistency of this fix. :(

What tests are you going to run for verification?

Thanks,
Serguei
*
*On 9/30/13 4:45 AM, Fredrik Arvidsson wrote:

Hi

Please help me to review the changes below:

Jira case: https://bugs.openjdk.java.net/browse/JDK-8024423
Webrev: 
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/ 



In this review I still have an outstanding unanswered question about 
the need to synchronize the access to the:

ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
and
ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);

calls in
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html
  


Please give me advise on this.

There will be a review soon regarding re-enabling the tests that failed because 
of the above bug, see:
https://bugs.openjdk.java.net/browse/JDK-8025576

Cheers
/Fredrik

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-02 Thread Fredrik Arvidsson 

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested and 
here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 



Regarding the need to synchronize the access to ClassLoaderDataGraph I 
have come to the conclusion that it should be safe to call this without 
any additional synchronization since newly loaded and added classes are 
appended to the end of its 'list' of classes. This would mean that the 
call could 'miss' the classes added during the collection phase, but 
this is not a big issue. I hope that my conclusion is correct.


I believe that the JvmtiGetLoadedClasses::getClassLoaderClasses(...) 
method is to be left alone this time since I got the impression that 
only SystemDictionary 'knows' about initiating class loaders.


There is plenty of room for improvement and simplification of much of 
the code in this area, but I feel that it must wait until another time. 
The task to re-factor and simplify would quickly grow out of hands I'm 
afraid :)


Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spit...@oracle.com wrote:

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*
2333   int src_index = 0;
2334   while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340 int hash_index = 0;
2341 while (hash_index < hash_len) {
2342   dest[dest_index++] = hash_buf[hash_index++];
2343 }
2344   }

The above can be simplified a little bit:
// Add the actual class name
for (int src_index = 0; src_index < src_length; ) {
  dest[dest_index++] = src[src_index++];
}
// If we have a hash, append it
for (int hash_index = 0; hash_index < hash_len; ) {
  dest[dest_index++] = hash_buf[hash_index++];
}

The conditionif(hash_len > 0)  is covered by the loop itself.

Style-related comments:
  -wrong indent at the lines: 2316-2319
  - need a space after the 'if' at the lines: 2315, 2339


*src/share/vm/prims/jvmtiGetLoadedClasses.cpp

*The copyright comment must be up-to-date.
The comments at the lines 35-38, 258-260 are not valid anymore.


> In this review I still have an outstanding unanswered question about
> the need to synchronize the access to the:
 
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);

> and
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);

This kind of walking is usually done at safepoint in a VM_Operation.
You will find many examples in the jvmtiEnv.cpp, for instance, 
VM_GetAllStackTraces.
The suggestion from Mikael is good too.

Should this fix include the getClassLoaderClasses() as well?
I'm still trying to realize the impact and consistency of this fix. :(

What tests are you going to run for verification?

Thanks,
Serguei
*
*On 9/30/13 4:45 AM, Fredrik Arvidsson wrote:

Hi

Please help me to review the changes below:

Jira case: https://bugs.openjdk.java.net/browse/JDK-8024423
Webrev: 
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/ 



In this review I still have an outstanding unanswered question about 
the need to synchronize the access to the:

ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
and
ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);

calls in
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html
  


Please give me advise on this.

There will be a review soon regarding re-enabling the tests that failed because 
of the above bug, see:
https://bugs.openjdk.java.net/browse/JDK-8025576

Cheers
/Fredrik

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-10-01 Thread serguei.spit...@oracle.com 

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*

2333   int src_index = 0;
2334   while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340 int hash_index = 0;
2341 while (hash_index < hash_len) {
2342   dest[dest_index++] = hash_buf[hash_index++];
2343 }
2344   }

The above can be simplified a little bit:
   // Add the actual class name
   for (int src_index = 0; src_index < src_length; ) {
 dest[dest_index++] = src[src_index++];
   }
   // If we have a hash, append it
   for (int hash_index = 0; hash_index < hash_len; ) {
 dest[dest_index++] = hash_buf[hash_index++];
   }

The conditionif(hash_len > 0)  is covered by the loop itself.

Style-related comments:
 -wrong indent at the lines: 2316-2319
 - need a space after the 'if' at the lines: 2315, 2339


*src/share/vm/prims/jvmtiGetLoadedClasses.cpp

*The copyright comment must be up-to-date.
The comments at the lines 35-38, 258-260 are not valid anymore.



In this review I still have an outstanding unanswered question about
the need to synchronize the access to the:


ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
and
ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);


This kind of walking is usually done at safepoint in a VM_Operation.
You will find many examples in the jvmtiEnv.cpp, for instance, 
VM_GetAllStackTraces.
The suggestion from Mikael is good too.


Should this fix include the getClassLoaderClasses() as well?
I'm still trying to realize the impact and consistency of this fix. :(

What tests are you going to run for verification?

Thanks,
Serguei
*
*On 9/30/13 4:45 AM, Fredrik Arvidsson wrote:


Hi

Please help me to review the changes below:

Jira case: https://bugs.openjdk.java.net/browse/JDK-8024423
Webrev: http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/ 



In this review I still have an outstanding unanswered question about 
the need to synchronize the access to the:

ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
and
ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);

calls in
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html
  


Please give me advise on this.

There will be a review soon regarding re-enabling the tests that failed because 
of the above bug, see:
https://bugs.openjdk.java.net/browse/JDK-8025576

Cheers
/Fredrik

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

2013-09-30 Thread Mikael Gerdin 

Fredrik,

On 09/30/2013 01:45 PM, Fredrik Arvidsson wrote:

Hi

Please help me to review the changes below:

Jira case: https://bugs.openjdk.java.net/browse/JDK-8024423
Webrev: http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/


In this review I still have an outstanding unanswered question about the
need to synchronize the access to the:

ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
and
ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);


It seems to me that the synchronization is only needed to make sure that 
we get a correct count for the _list array. New Klass*'s are pushed on 
the top of the _klasses list so that should not interfere with walking 
the Klass links.


One approach would be to do only one walk over the ClassLoaderDataGraph 
and use a growing datastructure instead of allocating one big linear 
array. I don't see any obvious requirement for a random-access 
datastructure so you could probably just put a Stack there and 
push the mirrors on the stack, get the stack size and put them on 
result_list in the correct order.


You can also make JvmtiGetLoadedClassesClosure a KlassClosure and get 
rid of the thread-local get_this/set_this since ClassLoaderDataGraph 
provides an interface for a KlassClosure as well as a callback function 
pointer.


/Mikael



calls in
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html
  


Please give me advise on this.

There will be a review soon regarding re-enabling the tests that failed because 
of the above bug, see:
https://bugs.openjdk.java.net/browse/JDK-8025576

Cheers
/Fredrik