RFR(s): 8191859: SoftReference clock/timestamp should be declared volatile

[Per Liden]

Hi Martin,

On 2017-11-24 23:56, Martin Buchholz wrote:
> These fields have been this way for a very long time, probably
> intentionally non-volatile.
> Is there an observable problem being solved here?

Sorry for not being more clear about that. Having them non-volatile can 
lead to the timestamp going backwards (e.g. in C2, if a clock load is 
cached over a safepoint), which in turn can lead to incorrect decision 
being made by the GC SoftRef policy about whether the referent should be 
cleared or not. While an incorrect decision is sub-optimal, it's not 
catastrophic. Worst case is that we clear a soft ref per-maturely.

However, after some discussions here we realized that the suggested fix 
isn't enough to prevent this from happening, e.g. when the interpreter 
is running without thread local safepoint polling. A robust fix for this 
would likely need a CAS to prevent the timestamp from going backwards.

For these reasons, I'm withdrawing my suggested change for now. I need 
to think more about the implication of a robust fix, or if we should 
just live with this issue.

cheers,
Per

> I see that out-of-thin-air longs are theoretically possible, but very
> rare in practice on current platforms.
> If all you're trying to do is to ensure atomicity and that the
> reads/writes don't get optimized away by the jit, then consider using
> "opaque" VarHandles instead.
>
> I was surprised there was no bug subcomponent for java.lang.ref
>
> On Fri, Nov 24, 2017 at 5:13 AM, Per Liden <per.liden at oracle.com
> <mailto:per.liden at oracle.com>> wrote:
>
>     The clock and timestamp fields in SoftReference should be declared
>     volatile. These fields are read or updated by the VM, (possibly)
>     concurrently with the execution of Java threads.
>
>     To be more specific, the timestamp field is read by the GC during
>     reference discovery, e.g. during G1/CMS concurrent mark. The clock
>     is updated by the GC, typically after reference processing has
>     completed.
>
>     Webrev: http://cr.openjdk.java.net/~pliden/8191859/webrev.0/
>     <http://cr.openjdk.java.net/~pliden/8191859/webrev.0/>
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8191859
>     <https://bugs.openjdk.java.net/browse/JDK-8191859>
>
>     /Per
>
>