RFR: 8364819: Post-integration cleanups for JDK-8359820 [v2]
Aleksey Shipilev
shade at openjdk.org
Thu Aug 7 10:51:14 UTC 2025
On Thu, 7 Aug 2025 08:28:40 GMT, David Holmes <dholmes at openjdk.org> wrote:
>> I am pretty sure the memory consistency here is irrelevant, given that: a) the `Thread` in question is likely already initialized for a long time, so it is unlikely we will lose anything release-acquire-wise; b) we only use these for pointer comparisons.
>>
>> But since we are here, and in the interest of clarity and avoiding future surprises, we can just summarily wrap these with `Atomic::release_store` and `Atomic::load_acquire` to be extra paranoidly safe. This is a failing path, so we don't care about performance, and would like to avoid a secondary crash in error handler some time in the future, if anyone reads anything from these threads.
>
> @shipilev The memory consistency is about seeing the writes to these fields in the thread that is signalled. acquire/release has no meaning in this context. I would not add acquire/release just in case someone in the future actually tried to follow those pointers - they are only for identifying purposes. If you are worried about that then lets go back to changing them to a value (intptr_t) so it will never be an issue.
It always feels dubious to me to add fences for the single-variable "visibility" reasons. The fences order things relative to other things, not just "flush" or "make this single variable visible". But that's a theoretical quibble. A more pressing problem is reading the thread marker without any acquire/relaxed semantics; _that_ might miss updates as well.
Given how messy C++ memory model is, and how data races are UB-scary there, I think we should strive to do `Atomic`-s as the matter of course for anything that transfers data between threads. Adjusting `Atomic::release_store` -> `Atomic::release_store_fence` would have satisfied both flavors of concurrency paranoia we are having, I think: it is equivalent to having a fence after the store, and it clearly makes Atomic release->acquire chain as well :)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26656#discussion_r2259902716
More information about the hotspot-dev
mailing list