RFR: 8317350: Move code cache purging out of CodeCache::UnloadingScope

[Thomas Schatzl]

On Mon, 16 Oct 2023 13:28:03 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Hi all,
>> 
>>   please review this refactoring that moves actual code cache flushing/purging out of `CodeCache::UnloadingScope`. Reasons:
>> 
>> * I prefer that a destructor does not do anything substantial - in some cases, 90% of time is spent in the destructor in that extracted method (due to https://bugs.openjdk.org/browse/JDK-8316959)
>> * imho it does not fit the class which does nothing but sets/resets some code cache unloading behavior (probably should be renamed to `UnloadingBehaviorScope` too in a separate CR).
>> * other existing methods at that level are placed out of that (or any other) scope object too - which is already the case for when doing concurrent unloading.
>> * putting it there makes future logging of the various phases a little bit easier, not having `GCTraceTimer` et al. in various places.
>> 
>> Testing: gha
>> 
>> Thanks,
>>   Thomas
>
> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 2068:
> 
>> 2066: 
>> 2067:     // Release unloaded nmethods's memory.
>> 2068:     CodeCache::flush_unlinked_nmethods();
> 
> The fact that recycling resources for nmethods involves two steps, unlink and free-unlinked, seems an insignificant impl detail in this caller context. Can that be hidden away from the public API, e.g. `do_unloading` perform these two steps directly?
> 
> Is there a reason why `flush_unlinked_nmethods` is outside the scope?

It is maybe an insignificant detail in the context of stw collectors, but for gcs doing concurrent class/code unloading there needs to be a handshake/memory synchronization between unlinking and freeing, so these two operations need to be split.

I.e. what they do at a high level is:

  unlink classes
  unlink code cache
  unlink other stuff

  handshake

  free unlinked classes
  free unlinked code
  free other stuff

Similarly I would like to split G1 class unloading into a STW part (unlinking stuff) and make all the freeing concurrent to avoid the additional (mostly) implementation overhead. Or at least start with that and then see if it is worth making everything concurrent. (The unlinking can be trimmed down further afaics).

Back to this CR: as the description states I think the current code wrongly hides this free-unlinked procedure in that `UnloadingScope` (there is a reason it is only used for the STW collectors) unnecessarily making the observed behavior (of one of the most time-consuming parts of class/code unloading) surprising.

Putting it on this level also allows more straightforward logging.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16011#discussion_r1360764085