[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR: 8297186: G1 triggers unnecessary full GCs when heap utilization is low [v3]
From:       Kim Barrett <kbarrett () openjdk ! org>
Date:       2022-11-30 17:38:26
Message-ID: nmxFuK9t1apwZfERwZjwRYXpVENykrWtB3d3kEk3sx0=.5667074d-b464-4ef1-8ed5-68c80f7b22cd () github ! com
[Download RAW message or body]

On Wed, 23 Nov 2022 09:50:32 GMT, Thomas Schatzl <tschatzl@openjdk.org> wrote:

> > Hi all,
> > 
> > can I have reviews for this change that better enforces that after gc there is at \
> > least one eden region to allocate into? 
> > That avoids the problem described in the PR.
> > 
> > Thanks,
> > Thomas
> 
> Thomas Schatzl has updated the pull request incrementally with one additional \
> commit since the last revision: 
> Improve test avoiding heap fragmentation so that it succeeds

Changes requested by kbarrett (Reviewer).

src/hotspot/share/gc/g1/g1Policy.cpp line 316:

> 314:     log_trace(gc, ergo, heap)("Young target length: Desired young length \
>                 smaller than allocated after GC. Allowing one extra eden region.");
> 315:     desired_young_length = allocated_young_length + 1;
> 316:   }

I don't understand this.  The log message doesn't match the test being performed (log \
says "smaller" but the test is for `<=`).  And the action taken seems kind of \
mysterious.  I think it's to force us into the `else` clause of the following `if` \
statement.  It might be more obvious if instead of this the `if` condition was \
changed to `(!after_gc && allocated_young_length >= desired_young_length)`, though \
then there is some additional stuff to do in the `else` branch too.  A comment \
instead would help.

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

PR: https://git.openjdk.org/jdk/pull/11209


[prev in list] [next in list] [prev in thread] [next in thread]