RFR: 8212989: Allow CompiledMethod ExceptionCache have unloaded klasses

[Erik Österlund]

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/

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