RFR: 8212989: Allow CompiledMethod ExceptionCache have unloaded klasses

[Per Liden]

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.

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