On Fri, 23 Aug 2024 21:02:25 GMT, Gerard Ziemski <gziem...@openjdk.org> wrote:
> Is the plan to check-in the fix with both paths? Or are we going to remove > the linked-list based one after the review? The plan is to have both versions available at run-time. In this plan, we will add JVM options to let the user to select either of them. This can be used for: 1️⃣ stepping back if new version fails in some use-cases, and also for 2️⃣ running benchmarks and comparing the results of both. > I still haven't run the benchmarks to verify the speed increase promise. > Ideally, I would like to do this with my mechanism, but will only do it, if I > can manage it without getting sucked into working on the mechanism itself. > > I will definitively want to run the provided microbenchmarks though. In `test_vmt_with_tree.cpp`, two versions are compared. One of the tests (`PerformanceComparison`) is comparing the time they take for the same operation. This test is skipped for now, because its expectation of time-improvement fails. Until the time we still did not merge all other related changes (like the `copy_flag` or `use_tag_inplace`, [#20330](https://github.com/openjdk/jdk/pull/20330)) to this PR we cannot expect the time to be improved. > src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 279: > >> 277: "Cannot commit verification bitmap >> memory"); >> 278: } >> 279: MemTracker::record_virtual_memory_type(verify_bitmap.base(), >> verify_bitmap..size(), mtGC); > > Does this even compile? No it doesn't, you're right. Shenandoah is not in the main build. I thought that GHA builds do this but seems not. Fixed. Thanks. > src/hotspot/share/nmt/memReporter.cpp line 440: > >> 438: if (all_committed) { >> 439: if (MemTracker::is_using_sorted_link_list()) { >> 440: CommittedRegionIterator itr = >> reserved_rgn->iterate_committed_regions(); > > Indentation. Done. > src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 94: > >> 92: if (si._stack_index < 0 || si._stack_index >= _stacks.length()) { >> 93: return _fake_stack; >> 94: } > > Is that a leftover from debugging? > > Shouldn't this be an `assert` in final code? You are right. Since the next lines check the `-1` as special case; this check should be either an `assert` or logged. I wait for @jdksjolen, as this is his code. > src/hotspot/share/nmt/nmtTreap.hpp line 219: > >> 217: } >> 218: last_seen = node; >> 219: return true; > > Why are we returning a `bool` value in `void` function? That is returning from the lambda function. In `visit_in_order(F func)`, when function `func` (in our case the lambda) returns `false` it means the iteration should stop. Otherwise, if it returns `true` it means to continue iterating. > src/hotspot/share/nmt/nmtTreap.hpp line 320: > >> 318: } >> 319: head = to_visit.pop(); >> 320: if(!f(head)) > > Needs space `if (!f(head))` Fixed. > src/hotspot/share/nmt/nmtTreap.hpp line 348: > >> 346: const int cmp_to = COMPARATOR::cmp(head->key(), to); >> 347: if (cmp_from >= 0 && cmp_to < 0) { >> 348: if(!f(head)) > > Needs space `if (!f(head))` Done. > src/hotspot/share/nmt/nmtTreap.hpp line 356: > >> 354: head = nullptr; >> 355: } >> 356: > > Remove. Done ------------- PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2309507439 PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2378923225 PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1778342931 PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1713348422 PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722829439 PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1778361486 PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722821927 PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722822141 PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1722822343
