On Mon, 5 Jun 2023 12:46:58 GMT, Kelvin Nilsen <[email protected]> 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
