On Wed, 11 Sep 2024 14:03:17 GMT, Gerard Ziemski <gziem...@openjdk.org> wrote:
>> Gerard Ziemski has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Coleen's feedback
>
> Are we sure we want `mt` for non-type parameter name in templates? We have
> these existing patterns already in our code:
>
>
> src/hotspot/share/utilities/growableArray.hpp:803:template <typename E,
> MemTag MT>
> src/hotspot/share/utilities/stack.hpp:54:template <class E, MemTag MT> class
> StackIterator;
> src/hotspot/share/utilities/concurrentHashTable.inline.hpp:78:template
> <typename CONFIG, MemTag MT>
> src/hotspot/share/utilities/chunkedList.hpp:31:template <class T, MemTag MT>
> class ChunkedList : public CHeapObj<MT>
> src/hotspot/share/gc/g1/g1BatchedTask.hpp:32:template <typename E, MemTag MT>
> src/hotspot/share/gc/shared/taskqueue.hpp:119:template <unsigned int N,
> MemTag MT>
> src/hotspot/share/gc/shared/taskqueue.hpp:327:template <class E, MemTag MT,
> unsigned int N = TASKQUEUE_SIZE>
> src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.hpp:40:template<class E,
> MemTag MT, unsigned int N = TASKQUEUE_SIZE>
> src/hotspot/share/nmt/arrayWithFreeList.hpp:34:template<typename E, MemTag MT>
>
>
> With mt they would look like:
>
>
> src/hotspot/share/utilities/growableArray.hpp:803:template <typename E,
> MemTag mt>
> src/hotspot/share/utilities/stack.hpp:54:template <class E, MemTag mt> class
> StackIterator;
> src/hotspot/share/utilities/concurrentHashTable.inline.hpp:78:template
> <typename CONFIG, MemTag mt>
> src/hotspot/share/utilities/chunkedList.hpp:31:template <class T, MemTag mt>
> class ChunkedList : public CHeapObj<mt>
> src/hotspot/share/gc/g1/g1BatchedTask.hpp:32:template <typename E, MemTag mt>
> src/hotspot/share/gc/shared/taskqueue.hpp:119:template <unsigned int N,
> MemTag mt>
> src/hotspot/share/gc/shared/taskqueue.hpp:327:template <class E, MemTag mt,
> unsigned int N = TASKQUEUE_SIZE>
> src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.hpp:40:template<class E,
> MemTag mt, unsigned int N = TASKQUEUE_SIZE>
> src/hotspot/share/nmt/arrayWithFreeList.hpp:34:template<typename E, MemTag mt>
>
>
> So `MT` or `mt` for non-type parameter name in templates, or should I punt on
> this particular change and leave it for a followup?
> Thank you @gerard-ziemski, for this huge change. After this change, the code
> looks much more nicer and consistent.
>
> If we are insisting on replacing `flag` with `tag`, I could find these missed
> ones by regexp search for `mem.*flag`:
>
> 7 results - 5 files
>
> Source root • src/hotspot/share/nmt/memMapPrinter.cpp: `83: // A Cache that
> correlates range with MEMFLAG, optimized to be iterated quickly`
>
> Source root • src/hotspot/share/nmt/memTracker.hpp: `208: // memory flags of
> the original region.`
>
> Source root • src/hotspot/share/nmt/vmatree.hpp:
>
> `97: assert(!(type == StateType::Released) || data.mem_tag == mtNone,
> "Released type must have flag mtNone");`
>
> `108: return static_cast<MemTag>(type_flag[1]);`
>
> Source root • test/hotspot/gtest/nmt/test_nmt_reserved_region.cpp: `50:
> ASSERT_EQ(region2.mem_tag(), mtThreadStack); // Should be correct flag`
>
> Source root • test/hotspot/gtest/nmt/test_vmatree.cpp:
>
> `435: const MemTag candidate_flags[candidates_len_flags] = {`
>
> `459: const MemTag mem_tag = candidate_flags[os::random() %
> candidates_len_flags];`
Thank you Ashfin for laking such a detailed look and providing the feedback.
I fixed the issues, except:
`108: return static_cast<MemTag>(type_flag[1]);`
This actually has more than just MemTag, it also has StateType, so I left it as
is.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/20872#issuecomment-2346602184
