RFR: 8311248: Refactor CodeCache::initialize_heaps to simplify adding new CodeCache segments [v9]

[Tobias Hartmann]

On Thu, 7 Mar 2024 10:58:19 GMT, Boris Ulasevich <bulasevich at openjdk.org> wrote:

>> These changes clean up the logic and the code of allocating codecache segments and add more testing of it, to open a door for further optimization of code cache segmentation.  The goal was to keep the behavior as close to the existing behavior as possible, even if it's not quite logical.
>> 
>> Also, these changes better account for alignment - PrintFlagsFinal shows the final aligned segment sizes, and the segments fill the ReservedCodeCacheSize without gaps caused by alignment.
>
> Boris Ulasevich has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits:
> 
>  - minor updates according to review comments
>  - minor update: align_up for ReservedCodeCacheSize
>  - one another cleanup round
>  - minor update. removed helper function as it caused many comments in the review
>  - set_size_of_unset_code_heap
>  - minor update
>  - apply suggestions
>  - cleanup & test udpdate
>  - 8311248: Refactor CodeCache::initialize_heaps to simplify adding new CodeCache segments

Looks good to me otherwise. I'll run some testing and report back once it passed.

src/hotspot/share/code/codeCache.cpp line 180:

> 178: GrowableArray<CodeHeap*>* CodeCache::_allocable_heaps = new(mtCode) GrowableArray<CodeHeap*> (static_cast<int>(CodeBlobType::All), mtCode);
> 179: 
> 180: static void check_min_size(const char *codeheap, size_t size, size_t required_size) {

Suggestion:

static void check_min_size(const char* codeheap, size_t size, size_t required_size) {

src/hotspot/share/code/codeCache.cpp line 183:

> 181:   if (size < required_size) {
> 182:     log_debug(codecache)("Code heap (%s) size " SIZE_FORMAT " below required minimal size " SIZE_FORMAT,
> 183:                          codeheap, size, required_size);

Should `size` and `required_size` be printed in `K`?

src/hotspot/share/code/codeCache.cpp line 198:

> 196: static void set_size_of_unset_code_heap(CodeHeapInfo* heap, size_t available_size, size_t used_size, size_t min_size) {
> 197:   assert(!heap->set, "sanity");
> 198:   heap->size = (available_size > used_size + min_size) ? (available_size - used_size) : min_size;

Suggestion:

  heap->size = (available_size > (used_size + min_size)) ? (available_size - used_size) : min_size;

src/hotspot/share/code/codeCache.cpp line 256:

> 254:   if (total != cache_size && !cache_size_set) {
> 255:     log_info(codecache)("ReservedCodeCache size %lld changed to total segments size NonNMethod %lld NonProfiled %lld Profiled %lld = %lld",
> 256:                         (long long) cache_size, (long long) non_nmethod.size, (long long) non_profiled.size,

Any reason you are using `%lld` here and below instead of `SIZE_FORMAT`?

src/hotspot/share/memory/virtualspace.cpp line 326:

> 324: ReservedSpace ReservedSpace::partition(size_t offset, size_t partition_size, size_t alignment) {
> 325:   assert(offset + partition_size <= size(), "partition failed");
> 326:   ReservedSpace result(base()+offset, partition_size, alignment, page_size(), special(), executable());

Suggestion:

  ReservedSpace result(base() + offset, partition_size, alignment, page_size(), special(), executable());

-------------

Changes requested by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17244#pullrequestreview-1975505901
PR Review Comment: https://git.openjdk.org/jdk/pull/17244#discussion_r1548916881
PR Review Comment: https://git.openjdk.org/jdk/pull/17244#discussion_r1548917691
PR Review Comment: https://git.openjdk.org/jdk/pull/17244#discussion_r1548919062
PR Review Comment: https://git.openjdk.org/jdk/pull/17244#discussion_r1548916641
PR Review Comment: https://git.openjdk.org/jdk/pull/17244#discussion_r1548923421