On Wed, 24 Jun 2026 10:25:09 GMT, Aleksey Shipilev <[email protected]> wrote:
>> While following up on concurrent marking performance, I noticed that we >> stopped / failed to inline some of the hot methods in marking loop. We need >> to rework this. >> >> This PR replaces the build-time GCC-specific "bump" for inlining heuristics >> into explicit inlining hints across the hot path. I have eyeballed the >> profiles on typical workloads and the inlining makes sense now. >> >> Additional testing: >> - [x] Linux x86_64 server fastdebug, `hotspot_gc_shenandoah` >> - [x] Ad-hoc marking performance tests >> - [x] Regular testing pipelines >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Aleksey Shipilev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Keep "inline" on do_chunked_array > - Merge branch 'master' into JDK-8385643-shenandoah-rework-mark-inline > - Cosmetics > - Benchmarks show array path is still important > - A few more cosmetics > - Touchup > - Work Looks good to me. Two things: - You might want to drop the `virtual` and make the methods `final` in two cases. - Several definitions still read `inline`. You might either also change them to `ALWAYS_INLINE` for consistency or, even better, drop any `inline` keywords on the definitions - they are redundant. (I know we haven't been consistent about it in the past, but maybe we should start now? ;-) ). I'll leave all of that to you. No need for re-review if you decide to make those changes. src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp line 106: > 104: > 105: ALWAYSINLINE > 106: virtual void do_oop(narrowOop* p) { do_oop_work(p); } I would drop the `virtual` here (it is redundant), and make the method `final` instead. src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp line 109: > 107: > 108: ALWAYSINLINE > 109: virtual void do_oop(oop* p) { do_oop_work(p); } Same here. src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 114: > 112: > 113: template <ShenandoahGenerationType GENERATION> > 114: inline void ShenandoahMark::count_liveness(ShenandoahLiveData* > live_data, oop obj, Klass* klass, uint worker_id) { The `inline` here is redundant (pre-existing, but you might want to clean this up while you're here). src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 158: > 156: > 157: template <class T> > 158: inline void > ShenandoahMark::do_chunked_array_start(ShenandoahObjToScanQueue* q, T* cl, > oop obj, Klass* klass, bool weak) { Redundant `inline` again. src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 225: > 223: > 224: template <class T> > 225: inline void ShenandoahMark::do_chunked_array(ShenandoahObjToScanQueue* > q, T* cl, oop obj, int chunk, int pow, bool weak) { Redundant `inline` again. src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 294: > 292: > 293: template<class T, ShenandoahGenerationType GENERATION> > 294: inline void ShenandoahMark::mark_through_ref(T *p, > ShenandoahObjToScanQueue* q, ShenandoahObjToScanQueue* old_q, > ShenandoahMarkingContext* const mark_context, bool weak) { Redundant `inline` again. src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 342: > 340: > 341: template<class T> > 342: inline void ShenandoahMark::mark_non_generational_ref(T* p, > ShenandoahObjToScanQueue* q, Redundant `inline` again. src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 357: > 355: } > 356: > 357: inline void ShenandoahMark::mark_ref(ShenandoahObjToScanQueue* q, Redundant `inline` again. src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp line 164: > 162: // upgraded to strong. In this case, this method also returns true. > 163: ALWAYSINLINE > 164: bool mark_strong(HeapWord* w, bool& was_upgraded); The *definition* in shenandoahMarkBitMap.inline.hpp still carries `inline`, which is redundant. src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp line 171: > 169: // strong or weak. > 170: ALWAYSINLINE > 171: bool mark_weak(HeapWord* heap_addr); The *definition* in shenandoahMarkBitMap.inline.hpp still carries `inline`, which is redundant. src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp line 59: > 57: */ > 58: ALWAYSINLINE > 59: bool mark_strong(oop obj, bool& was_upgraded); The *definition* in shenandoahMarkingContext.inline.hpp still carries `inline`, which is redundant. src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp line 62: > 60: > 61: ALWAYSINLINE > 62: bool mark_weak(oop obj); The *definition* in shenandoahMarkingContext.inline.hpp still carries `inline`, which is redundant. src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.inline.hpp line 35: > 33: > 34: template <class E, MemTag MT, unsigned int N> > 35: inline bool BufferedOverflowTaskQueue<E, MT, N>::pop(E &t) { This is redundant. src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.inline.hpp line 50: > 48: > 49: template <class E, MemTag MT, unsigned int N> > 50: inline bool BufferedOverflowTaskQueue<E, MT, N>::push(E t) { This is redundant. ------------- Marked as reviewed by rkennke (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/31634#pullrequestreview-4570122414 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473722200 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473722914 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473775534 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473778028 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473779264 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473814124 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473815146 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473816233 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473822191 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473823262 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473826113 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473827297 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473829420 PR Review Comment: https://git.openjdk.org/jdk/pull/31634#discussion_r3473831172
