RFR: 8212989: Allow CompiledMethod ExceptionCache have unloaded klasses

[Erik Osterlund]

Hi Vladimir,

Thanks for the review.

/Erik

> On 2 Nov 2018, at 23:49, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
>> On 11/1/18 3:14 PM, Erik Österlund wrote:
>> Hi Vladimir,
>> Thanks for the quick reply.
>>> On 2018-11-01 21:14, Vladimir Kozlov wrote:
>>> Hi Erik,
>>> 
>>> It seems these  changed are not based on latest sources. I don't see call to CodeCache::do_unloading in CodeCache::do_unloading() which is present in latest code:
>>> 
>>> http://hg.openjdk.java.net/jdk/jdk/file/5ea020bcaa0d/src/hotspot/share/code/codeCache.cpp#l683
>> Yeah this code is taken from the context of the future. Here is the exact same patch rebased against jdk-jdk:
>> http://cr.openjdk.java.net/~eosterlund/8212989/webrev.01/
>> And new webrev with your suggestions:
>> http://cr.openjdk.java.net/~eosterlund/8212989/webrev.02/
> 
> Looks good.
> 
>> And incremental of that to the jdk-jdk rebase:
>> http://cr.openjdk.java.net/~eosterlund/8212989/webrev.01_02/
>>> 
>>> May be make some methods private, like CompiledMethod::exception_cache_acquire().
>> Sure. Made the accessors protected, as some are used in nmethod too and it seems fine for subclasses to access.
>>> It seems you remove the only case when release_set_exception_cache() is called.
>> That is true. Removed.
>>> I don't see where purge_exception_caches() is used.
>> It's only called by a future version of ZGC from its new concurrent class unloading feature that is currently baking in the ZGC repository. The release_exception_cache member function deletes entries synchronously if cleanup is triggered in a safepoint (all GCs except ZGC), and otherwise defers deletion to the purge_exception_caches() method, that is expected to happen one global handshake operation after the exception caches have been unlinked by cleanup.
>>> Next sentence seems incomplete:
>>> 
>>> +  // unlinked. That is also when the CodeCache::exception_cache_free_list()
>> Fixed.
>>> Why you use exception_cache_acquire() in clean_exception_cache() and simple exception_cache() in add_exception_cache_entry()?
>> The reason is that in the context of insertion, you are holding the lock protecting against concurrent insertions. Therefore, entries you load are either entries that were protected by the same lock (other insertions), or entries that are as old or older than entries that have died a whole safepoint operation earlier (due to prepending inserts), which makes its memory state stable. Therefore, no acquire is required when reading the head in the insertion path.
> 
> Got it.
> 
> Thanks,
> Vladimir
> 
>> Thanks,
>> /Erik
>>> Thanks,
>>> Vladimir
>>> 
>>>> On 11/1/18 5:58 AM, Erik Österlund wrote:
>>>> Hi,
>>>> 
>>>> The ExceptionCaches of compiled methods is traditionally pruned in safepoints during class unloading. This allows the exception cache to have lock-free reads, while performing inserts under a lock, and pruning under a safepoint. With concurrent class unloading, the exception caches need to be cleaned concurrently.
>>>> 
>>>> To retain the lock-free reads that seem good to keep lock-free, a lock-free cleaning mechanism was introduced. Only one thread per compiled method cleans each exception cache, but that thread races with both single writers (under the ExceptionCache_lock) and any number of readers.
>>>> 
>>>> The head of the exception cache list is concurrently cleaned by both inserting threads and cleaning threads. This allows having an invariant that no newly prepended entries ever produce next pointers to dead exception caches, that are concurrently removed from the list. As for the internal next pointers, they are only pruned by the one concurrent cleaning thread. This happens concurrent to reads that simply skip over dead entries as they have different Klass* pointers to the one being searched for.
>>>> 
>>>> The single concurrent cleanup thread does not delete things removed from the list straight away. Instead, they are placed on a purge list that is freed up after a subsequent global handshaking operation. That allows ABA-free CAS instructions in the lock-free paths, and allows safe concurrent reading of entries in the exception cache list.
>>>> 
>>>> Note that we already incorrectly "relied on memory_order_consume" for the head pointer. But it used a volatile load. Assuming volatile loads retain memory_order_consume semantics is not allowed in HotSpot runtime code, so I changed those loads to use acquire accordingly.
>>>> 
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8212989/webrev.00/
>>>> 
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8212989
>>>> 
>>>> Thanks,
>>>> /Erik