Re: request for review (M), 6458402: 3 jvmti tests fail with CMS and +ExplicitGCInvokesConcurrent

[Daniel D. Daugherty]

On 12/30/2010 4:03 PM, Keith McGuigan wrote:

My last request for a code review this year, I promise!

Happy New Year!

This fix adds the missing JVMTI GC callbacks for CMS:

Webrev: http://cr.openjdk.java.net/~kamg/6458402/webrev.03/

Very nicely done. You should ping Alan B. to see if he has
time to sanity check the JVM/TI tap map stuff...


src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
   No comments.

src/share/vm/gc_implementation/concurrentMarkSweep/vmCMSOperations.cpp
   No comments.

src/share/vm/gc_implementation/g1/vm_operations_g1.cpp
   No comments.

src/share/vm/gc_implementation/parallelScavenge/vmPSOperations.cpp
   No comments.

src/share/vm/gc_implementation/shared/vmGCOperations.cpp
   No comments.

src/share/vm/gc_implementation/shared/vmGCOperations.hpp
   No comments.

src/share/vm/memory/genCollectedHeap.cpp
   No comments.

src/share/vm/prims/jvmtiExport.cpp
   No Comments.

src/share/vm/prims/jvmtiExport.hpp
   There's no longer any guidance about how GC should use the
   the JvmtiGCMarker. Maybe such guidance is no longer needed.
   Update: no such guidance is needed anymore.

src/share/vm/prims/jvmtiImpl.cpp
   No comments.

src/share/vm/prims/jvmtiImpl.hpp
   No comments.

src/share/vm/prims/jvmtiTagMap.cpp
   Lines 561, 564 - looks like just a whitespace change, but I wanted
       to be sure. Probably shouldn't clean up whitespace when you
       have substantive changes in the same file; it confused me and
       it might confuse other folks.

   No other comments other than nice job with the tag map changes.

src/share/vm/prims/jvmtiTagMap.hpp
   No comments.

src/share/vm/runtime/globals.hpp
   Since this option was advertised in a JDK6-Update release notes,
   we'll need to figure out if something needs to be said in the
   JDK6-Update release notes when this version of HSX-20 get there.

src/share/vm/runtime/jniHandles.cpp
   JVM/TI tap maps no longer use weak references so this change
   caught me by surprise. I'm guessing that you were just looking
   for a place to "hang" the JvmtiTagMap::weak_oops_do() call.
   If you're going to keep the call here, you should probably add
   a comment so other readers aren't confused...