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...