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