RFR: 8322484: 22-b26 Regression in J2dBench-bimg_misc-G1 (and more) on Windows-x64 and macOS-x64

[Albert Mingkun Yang]

On Thu, 25 Jan 2024 16:22:59 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1YoungCollector.cpp line 544:
>> 
>>> 542:     assert(G1ThreadLocalData::pin_count_cache(thread).count() == 0, "must be flushed");
>>> 543:   }
>>> 544: #endif
>> 
>> It's not obvious to see this assert in this context. I wonder if it's cleaner (or more natural) if it's moved to `JavaThreadRetireTLABAndFlushLogs`, where we flush the cache.
>
> It was meant similarly to the call to `_g1h->verifier()->check_region_attribute_table()` call to verify post conditions for that GC phase.
> Putting it right after `JavaThreadRetireTLABAndFlushLogs` seems too close to the change to kind of make it kind of redundant - i.e. that task is small enough to understand it at a glance anyway (well, me at least).
> 
> Thinking again, it might be most useful to put this into a method (like mentioned `check_region_attribute_table`) and call it there to be able to use it elsewhere during debugging if needed.
> 
> Of course, we can also just remove this - what do you think?

Removing it is better, IMO.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17552#discussion_r1466669935