RFR: 8212989: Allow CompiledMethod ExceptionCache have unloaded klasses

[Erik Österlund]

Hi Per,

Thanks for the review.

On 2018-11-05 08:34, Per Liden wrote:
> Hi Erik,
> 
> On 11/1/18 11: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 to me. Just one minor nit:
> 
> src/hotspot/share/code/codeCache.cpp
> ------------------------------------
> 
> 912       ExceptionCache* free_list_head = 
> Atomic::load(&_exception_cache_purge_list);
> 
> May I suggest that we rename free_list_head to purge_list_head.

Ahh yes. Will do before pushing.

Thanks,
/Erik

> cheers,
> Per
> 
> 
>>
>> 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.
>>
>> 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