RFR: 8189748: More precise closures for WeakProcessor::weak_oops_do calls

[Stefan Karlsson]

Thanks for the reviews, Per and StefanJ.

Two small changes from their review:

diff --git a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp 
b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp
--- a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp
+++ b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp
@@ -4382,10 +4382,6 @@
 
g1_policy()->phase_times()->record_string_dedup_fixup_time(fixup_time_ms);
    }

-  // Verify that the usages of keep_alive didn't find new objects to copy.
-  assert(per_thread_states->state_for_worker(0)->queue_is_empty(),
-         "both queue and overflow should be empty");
-
    g1_rem_set()->cleanup_after_oops_into_collection_set_do();

    if (evacuation_failed()) {


I removed this assert that I added, because it isn't needed for this 
call site. This keep alive closure is a non-copying closure:
   // Non Copying Keep Alive closure
   class G1KeepAliveClosure: public OopClosure {


diff --git a/src/hotspot/share/memory/iterator.hpp 
b/src/hotspot/share/memory/iterator.hpp
--- a/src/hotspot/share/memory/iterator.hpp
+++ b/src/hotspot/share/memory/iterator.hpp
@@ -48,10 +48,10 @@
    virtual void do_oop(narrowOop* o) = 0;
  };

-class DoNothingClosure: public OopClosure {
+class DoNothingClosure : public OopClosure {
   public:
-  void do_oop(oop* p)       {}
-  void do_oop(narrowOop* p) {}
+  virtual void do_oop(oop* p)       {}
+  virtual void do_oop(narrowOop* p) {}
  };
  extern DoNothingClosure do_nothing_cl;


Some style clean-ups.

I consider these two changes trivial and will push the updated patch.

Thanks,
StefanK

On 2017-10-20 19:55, Stefan Karlsson wrote:
> Hi all,
> 
> Please review this patch to use more precise closures for the 
> WeakProcessor::weak_oops_do calls:
> 
> http://cr.openjdk.java.net/~stefank/8189748/webrev.00
> https://bugs.openjdk.java.net/browse/JDK-8189748
> 
> ------------------------------------------------------------------------
> WeakProcessor::weak_oops_do takes three closures:
> 
> * is_alive - checks if the oop* points to an alive object
> 
> * keep_alive - applied to all oop*s that point to live objects. It's 
> typically used by the GC to update forwarded pointers to the to the new 
> copy of object. The marking GCs pass in a closure that marks objects and 
> pushes references to the marking stacks.
> 
> * complete - the intention is to use this closure to scan objects copied 
> by keep_alive, or mark through objects passed to the marking stacks.
> 
> However, much of this work is unnecessary and actually misleading.
> 
> The marking GCs should never find unmarked objects when the keep_alive 
> closure is applied. So, there's actually no need to apply the keep_alive 
> closure, and therefore also not necessary to apply the complete closure. 
> The proposal is to change these calls to:
> WeakProcessor::weak_oops_do(is_alive, &do_nothing_cl);
> 
> The copying young GCs should never find objects that have not been 
> copied, only pointers that have not been forwarded to already copied 
> objects. Therefore, the complete closure isn't needed for these calls 
> either. As a first step we could change them to:
>   WeakProcessor::weak_oops_do(is_alive, keep_alive);
>   assert(verify that no objects where copied...)
> 
> The keep_alive closure is still needed to forward the pointers to the 
> copied objects, but later changes could change the keep_alive closure to 
> be more explicit and only do the forwarding.
> 
> ------------------------------------------------------------------------
> The patch uses and moves the DoNothingClosure to iterator.hpp.
> 
> ------------------------------------------------------------------------
> For ParallelScavenge the PSKeepAliveClosure has been changed to 
> PSScavengeRootsClosure. The latter is stricter and don't allow oop*s to 
> be visited twice. This is the closure used by the StringTable cleaning.
> 
> ------------------------------------------------------------------------
> Tested with hs-tier1, hs-tier2, hs-tier3.
> 
> Thanks,
> StefanK