RFR: 8277486: NMT: Cleanup ThreadStackTracker

[David Holmes]

On Tue, 23 Nov 2021 13:54:57 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

>>> _thread_counter is updated with ThreadCritical lock, but is read without the lock. Change to atomic update to ensure reader will not read stale value.
>> 
>> Use of atomic updates with MO_RELAXED will not affect whether an Atomic::load sees a stale value or not.
>
>> > _thread_counter is updated with ThreadCritical lock, but is read without the lock. Change to atomic update to ensure reader will not read stale value.
>> 
>> Use of atomic updates with MO_RELAXED will not affect whether an Atomic::load sees a stale value or not.
> 
> I thought we settled this in [PR 6065](https://github.com/openjdk/jdk/pull/6065), as you stated:
> 
>> I see now that the C++ "Modification Order" definition requires the write to the counter to be (for want of a better term) "immediately visible" to any subsequent read - so no stale value could be read.
> 
> If you are referring the inaccurate read of the counter w/o the lock, then yes, it is a general problem with NMT, where various counters are *not* strictly sync'd to avoid taking locks.

Sorry @zhengyu123 but every time I re-read the C++ memory model stuff I get a different take on exactly what different MO's provide. The write-read coherence in the modification order requires that the write happens-before the read. And the inter-thread happens-before ordering then states the five conditions, any of which, establish the happens-before ordering. But I do not see any of those conditions being met for the Atomic::load that does the read and the Atomic::inc or dec that last updated the counter. They do not synchronize-with each other (unlike C++ atomics), there is no dependency order and no obvious transitive happens-before or synchronizes-with ordering. The atomic load is AFAICS no different from a memory ordering or visibility perspective, to a plain load as it takes no MO parameter and is not specified to provide any specific MO semantics - unlike in C++ where the default is seq-cst. So unless the Atomic::inc/dec provide some form of full memory synchronization barrie
 r, the load need not see the most recent write.

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

PR: https://git.openjdk.java.net/jdk/pull/6504