RFR: 8250597: G1: Improve inlining around trim_queue

[Thomas Schatzl]

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
- messes up survivor predictions
- it typically does not help much because typically you first fill up 
young then old and in effect the evacuation code just slower.
- if evacuation failure is a common occurrence where this might matter, 
you have other issues
- makes the code more complicated

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

Thanks,
   Thomas