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

[Thomas Schatzl]

On Tue, 17 Oct 2023 07:31:51 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>Then, could Serial and Parallel use APIs that don't expose these details?

There is no such API. This change does not intend to expose such a new API too. Just moving the various phases of code/class unloading to the same level in the source code as apparent to me to keep surprises low (and simplify logging to be able to _see_ problems in the first place. Then we can fix them in subsequent PRs).

Also I would like to keep the existing structure of class/code unloading for all collectors for uniformity (e.g. separate unlinking from free-unlinking - maybe call it "purging" in the future?) - so that they will have the same structure and print the same logging messages in the same order in the future (at least for the STW collectors including G1).

Note that other collectors have exactly the same phases, and unlinking is separate from purging everywhere else too. They just re-implement some phases using their own code (doing some renames in the process).

Having both the same structure/phases and same (more comprehensive) logging output for more collectors will make troubleshooting much easier.

There is no renaming in this change, e.g. `CodeCache::do_unloading()` (="unlinking") as this method does not do the complete unloading either indicated by having this external `flush_unlinked_nmethods` (="purging"). This is not the change to do this imo. 
(Iirc `CodeCache::do_unloading()` still has does some purging in it anyway, but let's not digress too much here).

> For instance, move flush_unlinked_nmethods inside CodeCache::do_unloading, as it is used only by those collectors.

It could, but then for (future) logging you would have to pass `GCTraceTimer` as the common way to do timing in gc code into `do_unloading()` which is some compiler code. Not sure if this is what we want as it is imo awkward. It is imho better, cleaner and sufficient to have timing outside at least initially. I.e. I would at this point prefer the style of


{ // Phase x
  GCTraceTimer x("do phase X");
  do_phase_x();
}


as how to time can be heavily collector dependent too. I would at the end of all that refactoring see if there is some useful coalescing of the methods into helpers that can be done.
Maybe these scopes should be put into separate helper methods, I haven't decided yet what's best.

>Why is flush_unlinked_nmethods outside of UnloadingScope? This newly-introduced scope in the caller context seems extremely out of place, IMO.

I believe most of your questions stem from the naming. Please, do not be too hung up on names. The description of this PR already mentioned that `UnloadingScope` is a misnomer (and misnamed code is common in Hotspot code, or responsibilities changed over the course of years without fixing up the naming). So in addition to this class, be aware that there are lots of misnamed and not uniformly named methods in class/code unloading code too. `UnloadingScope` controls *unlinking* behavior by setting some globals and does not control the whole unloading process (ie. unlinking + purging).

Additionally `UnloadingScope` actually did _not_ really contain `flush_unlinked_nmethods` earlier either. It has been the last call in the destructor of the `UnloadingScope`, _outside_, after the unlinking behavior has been reset. This is even more strange if you think about it.

So from my understanding of the code this scope object ought to enclose only the unlinking part of the unloading (i.e. the decision of what to unlink, that is `CodeCache::do_unloading()`) before. This change only at most exposes the existing (as you say ugly - I agree) structure.

Which isn't a bad thing to me to have exposed. It's not in scope of this change to fix this imo because that would mix it with other unrelated changes.

(`UnloadingScope` should be called something different, maybe after this discussion something like `UnlinkingScope` or similar? Idk.)

>>  Putting it on this level also allows more straightforward logging.
>I don't get this. Can't see any log-print logic inside flush_unlinked_nmethods.

... in the future. (https://bugs.openjdk.org/browse/JDK-8315504)[https://bugs.openjdk.org/browse/JDK-8315504] intends to add more timing/logging for every phase.

There is currently no plan for comprehensive logging inside the various phases of the class/code unloading (at least not to the level of selected methods I did in recent investigations).

What I intend to do is revisiting the phases in the future and move around work to better reflect the unlinking/purging split, and also link them together with a real `UnloadingScope` covering the whole unloading process (not to be mixed up with the current `UnloadingScope`) to allow gc-specific replacements/optimizations of the phases.

Obviously there can be helper methods that simplify things for various gcs. This PR isn't this change though.

Hth,
  Thomas

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

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