RFR: 8317809: Insertion of free code blobs into code cache can be very slow during class unloading [v3]

[Albert Mingkun Yang]

On Fri, 24 Nov 2023 09:18:25 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Insert code blobs in a sorted fashion to exploit the finger-optimization when adding, making this procedure O(n) instead of O(n^2)
>> 
>> Introduces a globally available ClassUnloadingContext that contains common methods pertaining to class and code unloading. GCs may use it to efficiently manage unlinked class loader datas and nmethods to allow use of common methods (unlink/merge).
>> 
>> The steps typically are registering a new to be unlinked CLD/nmethod, and then purge its memory later. STW collectors perform this work in one big chunk taking the CodeCache_lock, for the entire duration, while concurrent collectors lock/unlock for every insertion to allow for concurrent users for the lock to progress.
>> 
>> Some care has been taken to stay consistent with an "unloading = unlinking + purge" scheme; however particularly the existing CLD handling API (still) mixes unlinking and purging in its CLD::unload() call. To simplify this change that is mostly geared towards separating nmethod unlinking from purging, to make code blob freeing O(n) instead of O(n^2).
>> 
>> Upcoming changes will
>> * separate nmethod unregistering from nmethod purging to allow doing that in bulk (for the STW collectors); that can significantly reduce code purging time for the STW collectors.
>> * better name the second stage of unlinking (called "cleaning" throughout, e.g. the work done in `G1CollectedHeap::complete_cleaning`)
>> * untangle CLD unlinking and what's called "cleaning" now to allow moving more stuff into the second unlinking stage for better parallelism
>> * G1: move some significant tasks from the remark pause to concurrent (unregistering nmethods, freeing code blobs and cld/metaspace purging)
>> * Maybe move Serial/Parallel GC metaspace purging closer to other unlinking/purging code to keep things local and allow easier logging.
>> 
>> These are the reason for the class hierarchy for `ClassUnloadingContext`: the goal is to ultimately have about this phasing (for G1):
>> 1. collect all dead CLDs, using the `register_unloading_class_loader_data` method *only*
>> 2. parallelize the stuff in `ClassLoaderData::unload()` in one way or another, adding them to the `complete_cleaning` (parallel) phase.
>> 3. `purge_nmethods`, `free_code_blobs` and the `remove_unlinked_nmethods_from_code_root_set` (from JDK-8317007) will be concurrent.
>> 
>> Particularly the split of `SystemDictionary::do_unloading` into "only" traversing the CLDs to find the dead ones and then in parallel process them in 2. a...
>
> Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch 'master' into mergeme
>  - iwalulya review, naming
>  - 8317809 Insert code blobs in a sorted fashion to exploit the finger-optimization when adding, making this procedure O(n) instead of O(n^2)
>    
>    Introduce a globally available ClassUnloadingContext that contains common methods pertaining to class and code unloading.
>    GCs may use it to efficiently manage unlinked class loader datas and nmethods to allow use of common methods (unlink/merge).
>    
>    The steps typically are registering a new to be unlinked CLD/nmethod, and then purge its memory later. STW collectors perform
>    this work in one big chunk taking the CodeCache_lock, for the entire duration, while concurrent collectors lock/unlock for every
>    insertion to allow for concurrent users for the lock to progress.
>    
>    Some care has been taken to stay consistent with an "unloading = unlinking + purge" scheme; however particularly the existing
>    CLD handling API (still) mixes unlinking and purging in its CLD::unload() call. To simplify this change that is mostly geared
>    towards separating nmethod unlinking from purging, to make code blob freeing O(n) instead of O(n^2).
>    
>    Upcoming changes will
>    * separate nmethod unregistering from nmethod purging to allow doing that in bulk (for the STW collectors); that can significantly
>      reduce code purging time for the STW collectors.
>    * better name the second stage of unlinking (called "cleaning" throughout, e.g. the work done in `G1CollectedHeap::complete_cleaning`)
>    * untangle CLD unlinking and what's called "cleaning" now to allow moving more stuff into the second unlinking stage for better
>      parallelism
>    * G1: move some signifcant tasks from the remark pause to concurrent (unregistering nmethods, freeing code blobs and cld/metaspace purging)
>    * Maybe move Serial/Parallel GC metaspace purging closer to other unlinking/purging code to keep things local and allow easier logging.
>  - Only run test case on debug VMs, sufficient
>  - 8320331 g1 full gc "during" verification accesses half-unloaded metadata

src/hotspot/share/gc/shared/classUnloadingContext.cpp line 120:

> 118:   for (uint i = 0; i < _num_nmethod_unlink_workers; ++i) {
> 119:     NMethodSet* set = _unlinked_nmethods[i];
> 120:     for (int j = 0; j < set->length(); ++j) {

I wonder if one can use range-based for loop here.

src/hotspot/share/gc/shared/classUnloadingContext.cpp line 171:

> 169:       ConditionalMutexLocker ml_inner(CodeCache_lock, _lock_codeblob_free_separately, Mutex::_no_safepoint_check_flag);
> 170:       CodeCache::free(nmethod_set->at(i));
> 171:     }

I feel it would be clearer if the for-loop is duplicated to handle either case separately.

src/hotspot/share/gc/shared/classUnloadingContext.hpp line 63:

> 61: };
> 62: 
> 63: class DefaultClassUnloadingContext : public ClassUnloadingContext {

I don't understand why they need to be two classes, even after reading "These are the reason for the class hierarchy for...". The reference to future/other PR(s) in the description doesn't really help -- it's unclear what is *necessary* for the current PR and what is preparation for future PR(s).

src/hotspot/share/gc/shared/classUnloadingContext.hpp line 95:

> 93: };
> 94: 
> 95: #endif // SHARE_GC_SHARED_CLASSUNLOADINGCONTEXT_HPP

Seems missing a newline.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16759#discussion_r1406262742
PR Review Comment: https://git.openjdk.org/jdk/pull/16759#discussion_r1406264755
PR Review Comment: https://git.openjdk.org/jdk/pull/16759#discussion_r1406282829
PR Review Comment: https://git.openjdk.org/jdk/pull/16759#discussion_r1406283473