RFR: 8287805: Shenandoah: consolidate evacuate-update-root closures

Aleksey Shipilev shade at openjdk.org
Wed Jun 22 15:30:01 UTC 2022


On Fri, 3 Jun 2022 19:00:01 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

> ShenandoahEvacuateUpdateMetadataClosure and ShenandoahEvacuateUpdateRootsClosure are mostly same, can be consolidated.
> 
> Also, only uses of ShenandoahEvacuateUpdateMetadataClosure all pass MO_UNORDERED template parameter, so it can be eliminated.
> 
> Test:
> 
> - [x] hotspot_gc_shenandoah

Changes requested by shade (Reviewer).

src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp line 95:

> 93: class ShenandoahEvacuateUpdateRootClosureBase : public ShenandoahOopClosureBase {
> 94: protected:
> 95:     ShenandoahHeap* const _heap;

Here and later the indenting is a bit off: needs to be 2 spaces, not 4.

src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp line 99:

> 97:     inline ShenandoahEvacuateUpdateRootClosureBase();
> 98:     inline virtual void do_oop(oop* p);
> 99:     inline virtual void do_oop(narrowOop* p);

Do we have to make these functions `virtual`? I supposed we do templated closures exactly to avoid any sort of virtual method dispatching. Maybe I am missing something here...

src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp line 144:

> 142: template <bool atomic>
> 143: void ShenandoahEvacuateUpdateRootClosureBase<atomic>::do_oop(oop* p) {
> 144:   do_oop_work(p, Thread::current());

Not fond of calling `Thread::current()` on this hotpath. From the code, it looks like we only need it when actually calling `_heap->evacuate_object()`, can we move it there? 

Related: is `ShenandoahContextEvacuateUpdateRootsClosure::_thread` used now? Can we use it on some paths too?

-------------

PR: https://git.openjdk.org/jdk/pull/9023



More information about the hotspot-gc-dev mailing list