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

[Boris Ulasevich]

On Mon, 22 Jan 2024 19:26:47 GMT, Evgeny Astigeevich <eastigeevich at openjdk.org> wrote:

>> Boris Ulasevich has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
>> 
>>   apply suggestions
>
> src/hotspot/share/code/codeCache.cpp line 260:
> 
>> 258:     non_profiled.set = true;
>> 259:     non_profiled.enabled = false;
>> 260:   }
> 
> It can be rewritten to the shorter version:
> 
> // If either heap1 or heap2 is not available, its size is added to the size of the available heap.
> static void reuse_unavailable_heap_size(CodeHeapInfo *heap1, CodeHeapInfo *heap2) {
>    assert(CodeCache::heap_available(heap1->type) || CodeCache::heap_available(heap2->type), "At least one code heap must be available");
>    if (!CodeCache::heap_available(heap1->type)) {
>      swap(heap1, heap2);
>    } else if (CodeCache::heap_available(heap2->type)) {
>      // Both code heaps are available. Nothing needs to be done.
>      return;
>    }
>    heap1->size += heap2->size;
>    heap2->size = 0;
>    heap2->set = true;
>    heap2->enable = false;
> }
> 
> 
> Now we can replace those two IFs with:
> 
> // For compatibility we add the size of an unavailable code heap to the size of the available heap.
> reuse_unavailable_heap_size(&non_profiled, &profiled);

That sounds reasonable. But that is kind of the opposite of what we are doing with this change. We are moving to a plain, simple, readable code.

> src/hotspot/share/code/codeCache.cpp line 264:
> 
>> 262:   size_t compiler_buffer_size = 0;
>> 263:   COMPILER1_PRESENT(compiler_buffer_size += CompilationPolicy::c1_count() * Compiler::code_buffer_size());
>> 264:   COMPILER2_PRESENT(compiler_buffer_size += CompilationPolicy::c2_count() * C2Compiler::initial_code_buffer_size());
> 
> We can move the code inside the following IF.
> We can move ` size_t non_nmethod_min_size = min_cache_size` before the IF and adjust it in the IF.
> This could be simplified with `CodeHeapInfo::min_size`.

size_t compiler_buffer_size = 0;
  COMPILER1_PRESENT(compiler_buffer_size += CompilationPolicy::c1_count() * Compiler::code_buffer_size());
  COMPILER2_PRESENT(compiler_buffer_size += CompilationPolicy::c2_count() * C2Compiler::initial_code_buffer_size());

  if (!non_nmethod.set) {
    non_nmethod.size += compiler_buffer_size;
  }

  ...

  // Compatibility.
  // Override Non-NMethod default size if two other segments are set explicitly
  size_t non_nmethod_min_size = min_cache_size + compiler_buffer_size;
  if (!non_nmethod.set && profiled.set && non_profiled.set) {
    non_nmethod.size = subtract_size(cache_size, profiled.size + non_profiled.size, non_nmethod_min_size);
  }

  ...

  check_min_size("non-nmethod code heap", non_nmethod.size, non_nmethod_min_size)


I do not support idea to move compiler_buffer_size setup under if (!non_nmethod.set).

> src/hotspot/share/code/codeCache.cpp line 272:
> 
>> 270:   if (!profiled.set && !non_profiled.set) {
>> 271:     non_profiled.size = profiled.size = (cache_size > non_nmethod.size + 2 * min_size) ?
>> 272:                                         (cache_size - non_nmethod.size) / 2 : min_size;
> 
> The code calculating default sizes is not simple. Are you sure your code produces the same results?

Yes, I am sure the logic is the same as before

> src/hotspot/share/code/codeCache.cpp line 281:
> 
>> 279:   if (!profiled.set && non_profiled.set) {
>> 280:     profiled.size = subtract_size(cache_size, non_nmethod.size + non_profiled.size, min_size);
>> 281:   }
> 
> `subtract_size` can do subtracting or nothing by return `min_size`. This is not obvious from its name.
> What about a function:
> 
> // Precondition: either or both of heaps must be set.
> //
> // If either of heaps size is not set, its size is set to max(available_size - set_heap.size, min_size).
> static void set_size_of_unset_code_heap(CodeHeapInfo *heap1, CodeHeapInfo *heap2, size_t available_size, size_t min_size) {
>   assert(...); //check precondition
>   if (heap1->set && heap2->set)
>     return;
> 
>   if (!heap2->set)
>     swap(heap1, heap2);
> 
>   heap1->size = (available_size > heap2->size + min_size) ? (available_size - heap2->size) : min_size;
> }
> 
> 
> Now we can unite two IFs into `else` case of `if (!profiled.set && !non_profiled.set)`:
> 
> if (!profiled.set && !non_profiled.set) {
>  ...
> } else {
>   set_size_of_unset_code_heap(&profiled, &non_profiled, cache_size -  non_profiled.size, min_size)
> }

Again. The idea is to move to a plain, simple, readable code, if you don't mind

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17244#discussion_r1474337319
PR Review Comment: https://git.openjdk.org/jdk/pull/17244#discussion_r1474337569
PR Review Comment: https://git.openjdk.org/jdk/pull/17244#discussion_r1474337735
PR Review Comment: https://git.openjdk.org/jdk/pull/17244#discussion_r1474337998