On Mon, 5 Jun 2023 12:46:58 GMT, Kelvin Nilsen <kdnil...@openjdk.org> wrote:
>> Kelvin Nilsen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove three asserts making comparisons between atomic volatile variables >> >> Though changes to the volatile variables are individually protected by >> Atomic load and store operations, these asserts were not assuring >> atomic access to multiple volatile variables, each of which could be >> modified independently of the others. The asserts were therefore not >> trustworthy, as has been confirmed by more extensive testing. > > src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 660: > >> 658: void ShenandoahGeneration::increase_used(size_t bytes) { >> 659: Atomic::add(&_used, bytes); >> 660: } > > Note that C++ subexpression evaluation order is undefined. Here is an > example of what can go wrong with the removed assertion: > > ThisThread: fetches _affiliated_region_count * begins to multiply with > region_size > OtherThread: increases _used and increases _affiliated_region_count > appropriately (for a large allocation) > ThisThread: fetches _used and observes assert violation We have seen this race manifest in actual testing, which is what has motivated us to remove the assertions. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1218045035