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

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

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

Stefan Karlsson
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
Reply | Threaded
Open this post in threaded view
|

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

Per Liden
Looks good.

/Per

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
Reply | Threaded
Open this post in threaded view
|

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

Stefan Johansson
In reply to this post by Stefan Karlsson
Looks good,
StefanJ

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

Reply | Threaded
Open this post in threaded view
|

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

Stefan Karlsson
In reply to this post by 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