RFR: 8335925: Serial: Move allocation API from Generation to subclasses [v2]

[Guoxiong Li]

On Mon, 15 Jul 2024 12:32:14 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Trivial moving methods from parent class to subclasses. The unused second arg is also removed along the way. The API names are descriptive enough so that the accompanying comments are dropped as well.
>
> Albert Mingkun Yang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into s1-gen-alloc
>  - s1-gen-alloc

I incline to agree this patch. @kimbarrett may think the common APIs should be placed in the parent class (`Generation` here).  But the method `DefNewGeneration::expand_and_allocate` confused me when I read code before. And the method `TenuredGeneration::par_allocate` has not been used which is removed in this patch.  So in this case, I consider it is good to move/remove them from parent class.

src/hotspot/share/gc/serial/defNewGeneration.cpp line 881:

> 879:   // We don't attempt to expand the young generation (but perhaps we should.)
> 880:   return allocate(size);
> 881: }

Previously, the virtual method `expand_and_allocate` is inherited from `Generation` so that we have such strange implementation. But now, you remove the related methods in `Generation`, I think you can remove the method `DefNewGeneration::expand_and_allocate` and use `DefNewGeneration::allocate` instead.

src/hotspot/share/gc/serial/defNewGeneration.hpp line 217:

> 215: 
> 216:   // Expand young-gen and invoke allocate above.
> 217:   HeapWord* expand_and_allocate(size_t size);

Please note the comment and implementation (shown below) in the method `expand_and_allocate`. I don't think it is good to add a comment which may be conflict with the comment below.


HeapWord* DefNewGeneration::expand_and_allocate(size_t size) {
  // We don't attempt to expand the young generation (but perhaps we should.)
  return allocate(size);
}


The comment `Expand young-gen and invoke allocate above` can be changed to something like `Intendedly expand young-gen and allocate object, but currently, we don't expand the young-gen in this method.`

Maybe we can remove `DefNewGeneration::expand_and_allocate`, see my comment in `DefNewGeneration::expand_and_allocate`.

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

PR Review: https://git.openjdk.org/jdk/pull/20084#pullrequestreview-2193892949
PR Review Comment: https://git.openjdk.org/jdk/pull/20084#discussion_r1688026363
PR Review Comment: https://git.openjdk.org/jdk/pull/20084#discussion_r1688005960