On Tue, 17 Nov 2020 10:16:28 GMT, Stefan Johansson <sjoha...@openjdk.org> wrote:
>> Thomas Schatzl has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains three commits: >> >> - Merge branch 'master' of https://git.openjdk.java.net/jdk into >> 8253081-null-narrow-klass-changes2 >> - kbarrett review >> - Initial import > > I've been focusing on the GC-parts and it looks good in general just a few > comments. Ran tier1-4, tier5-gc with the changes from ea78aa1 (contributed by @iklam after some discussion). > src/hotspot/share/gc/g1/g1FullGCPrepareTask.hpp line 60: > >> 58: G1FullGCCompactionPoint* _cp; >> 59: uint _humongous_regions_removed; >> 60: uint _open_archive_regions_freed; > > Since we no longer use these counters to update the sets, it is enough to > just have one counter to track if any region has been freed. Or use a boolean > if you prefer that. Fixed. > src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 4458: > >> 4456: heap_region_iterate(&cl); >> 4457: >> 4458: remove_from_old_gen_sets(0, 0, cl.humongous_regions_reclaimed()); > > Looking at this call and now having three parameters that are "optional" for > `remove_from_old_gen_sets()` I wonder if it would be cleaner to have three > functions, one for each set. It would increase the number of times we take > the look, but we could restructure the code in `G1ReclaimEmptyRegionsTask` to > not do the updates in the worker threads and that way only take the lock when > there will be no contention. > > If you fell like this is outside the scope of this change, please file an > issue instead. I am not very clear on what the problem there is and how the parameters are optional. I do not completely understand how adding an extra method just for this caller would improve the code significantly. I'll opt to defer this cleanup to a separate CR. ------------- PR: https://git.openjdk.java.net/jdk/pull/1163
