RFR: 8250597: G1: Improve inlining around trim_queue

[Kim Barrett]

> On Aug 14, 2020, at 5:18 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi,
> 
> On 13.08.20 15:48, Kim Barrett wrote:
>>> On Aug 13, 2020, at 7:16 AM, stefan.johansson at oracle.com wrote:
>>> 
>>> Hi Kim (and Thomas),
>>> 
>>> On 2020-08-07 09:57, Thomas Schatzl wrote:
>>>>>>> [...]
>>>>>>> CR:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8250597
>>>>>>> Webrev:
>>>>>>> https://cr.openjdk.java.net/~kbarrett/8250597/open.00/
>>>>>>> https://cr.openjdk.java.net/~kbarrett/8250597/open.01/
>>>>>>> https://cr.openjdk.java.net/~kbarrett/8250597/open.01.inc/
>>> 
>>> Looks good, nice refactoring.
>> Thanks.
>>>> The current code is nicer, yes. Thanks for refactoring this a bit. Also the changes in the allocation path where the old-gen-is-full
>>>> check has been moved (I did not check performance with this part of >>> the change).>>
>>> I looked at this an extra time as well. But if there would be any
>>> slight regression it would only be when we have an evac
>>> failure, right? So to me the cleaner code is more important.
>> Sorry I forgot to discuss this change in the RFR email.
>> The rationale is, as you surmised, to move uncommon work from the
>> normal fast path to slow path.
>> As a result, we'll now attempt the fast-path plab allocation when the
>> destination is old and old-gen is full. And that allocation might even
>> succeed, because we haven't necessarily tossed the plab after old-gen
>> full is detected. So we might squeeze a few more objects into the
>> plab. But usually it will be a waste of time in that uncommon
>> situation, but not a lot of time. And that seems like the right
>> trade-off to me.
>> And as you say, it does make the code cleaner.
>> Also, this might also set us up for a different fall-back. (I haven't
>> thought this all through yet.) If the source object is young and can't
>> be promoted to old because old is full, then try copying it a young
>> survivor region, if there are any, rather than going immediately to
>> evacuation failure. I think there's an RFE for that. I don't think we
>> want any overhead for that on the fast path, but here seems like a
>> good place for handling that situation. Something along this line
>> probably makes the code less clean...
> 
> This feature has been removed the last time that part of the allocation path has been reworked. The rationale has been that copying old objects into young

I wasn’t suggesting copying old objects into young.  That seems like it might have other
problems.  I was suggesting an attempt to copy young to old when old is full might instead
try copying to young instead.  But I don’t know if that really helps; if we’ve simply run out
of regions…

> - if evacuation failure is a common occurrence where this might matter, you have other issues

Indeed.

> I would still advise against doing so given that there are more important issues with evacuation failure still remaining.

I wasn’t planning to really look into doing anything of that sort any time soon.  I think there
may be other fish to fry on the fast path, plus some features like better splitting of arrays
into subtasks and the like.