On Tue, 31 Oct 2023 17:08:15 GMT, Gerard Ziemski <gziem...@openjdk.org> wrote:
>> Thomas Stuefe has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix various builds > > src/hotspot/share/nmt/memMapPrinter.cpp line 79: > >> 77: const void* const min = MAX2(from1, from2); >> 78: const void* const max = MIN2(to1, to2); >> 79: return min < max; > > I had to rewrite it as: > > `return MAX2(from1, from2) < MIN2(to1, to2);` > > to make sure I understand it, or better yet, why not have it as a macros? > > `#define RANGE_INTERSECT(min, max) (return MAX2(from1, from2) < MIN2(to1, > to2))` > > MAX2 and MIN2 are macros already and we're not doing anything fancy here. I'll do the former, that is clearer I agree, but leave the latter out (I assume with the macro you mean add it to globalDefenitions.hpp). I fear that a lot of bikeshedding and general discussions would start, and to do it properly needs a bit more time. Because we really would benefit from having a nice templatized utility classes for ranges. Like MemRegion, but that one is tied to HeapWord* I think. > src/hotspot/share/nmt/memMapPrinter.cpp line 89: > >> 87: // NMT deadlocks (ThreadCritical). >> 88: Range* _ranges; >> 89: MEMFLAGS* _flags; > > Why did you decide to have two separate memory chunks? > > I think I'd have used a struct to hold Range* and MEMFLAGS* and use that > struct in a single array. Two reasons: - more condensed, needs less memory. MEMFLAGS is a byte, Range is 16 bytes. A combined structure would be 24 bytes with padding (since the structure needs to be pointer aligned). We'd loose 7 bytes. - It is more cache friendly, hence faster. I need to iterate through all ranges, but only need to access the MEMFLAGS for the one range I find. By keeping the ranges condensed, more of them fit into a cache line, so iteration is faster. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1378475306 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1378476876
