RFR: 8214556: Crash in DependencyContext::remove_dependent_nmethod still happens

[Kim Barrett]

> On Dec 3, 2018, at 5:13 PM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Kim,
> 
> Thank you for reviewing this.
> 
> New full webrev:
> http://cr.openjdk.java.net/~eosterlund/8214556/webrev.01/
> 
> Incremental:
> http://cr.openjdk.java.net/~eosterlund/8214556/webrev.00_01/

Looks good, and I don't need a new webrev for fixing the claim_cleanup
comment mentioned below.

------------------------------------------------------------------------------
265 // We use a safepoint counter to track the safepoint counter the last time a given
266 // dependency context was cleaned. GC threads claim cleanup tasks by performing
267 // a CAS on this value.
268 bool DependencyContext::claim_cleanup() {

Comment needs to be updated.

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

Regarding the use of uint64_t as the epoch type (below), I *think*
wrapping is only a problem because of the comparison in claim_cleanup.
But I'm not sure why this is using a >= test rather than an == test; I
think there's only one cleaning pass in progress at a time (e.g.
cleaning_start/stop pairs never overlap). Even if there can be
several, the 1/2 range technique used by GlobalCounter would seem
applicable.

But in the interest of getting this bug fixed sooner (since it's
causing a lot of testing noise), I'm okay with leaving the counters as
uint64_t and discussing it offline.


> On 2018-12-03 22:44, Kim Barrett wrote:
>> src/hotspot/share/code/dependencyContext.hpp
>> 81   volatile uint64_t*       _last_cleanup_addr;
>> ...
>> 93   static uint64_t                _cleaning_epoch_monotonic;
>> 94   static volatile uint64_t       _cleaning_epoch;
>> Do these really need to be uint64_t? It seems like 64bit volatile
>> (with atomic accesses) could be costly or problematic for a 32bit
>> platform. (And the Atomic documentation says verification is
>> required.) Maybe uintx? Though even just uint seems like it might be
>> adequate. Oh, but a value of zero has special meaning. So if the
>> global monotonic counter does wrap (which seems entirely possible)
>> then an extra increment would be needed. Alternatively, start it at 1
>> and increment by 2?
>> (The _monotonic suffix should perhaps also be reconsidered if such a
>> change is made.  "latest"?)
> 
> I would like to keep this counter uint64_t. If wrapping happens, we are in a lot of trouble. All platforms support atomic load/store on uint64_t, including 32 bit platforms. It's other atomic operations like add-and-fetch that does not necessarily support that on 32 bit platforms. And I only need loads and stores.