RFR: 8320655: awt screencast robot spin and sync issues with native libpipewire api

[Anton Bobrov]

On Fri, 1 Dec 2023 13:44:21 GMT, Maxim Kartashev <mkartashev at openjdk.org> wrote:

>> @mkartashev hey, a fellow Sun comrade! (i still have my Sun badge too) :)
>> 
>> This is for similar reasons to the existing 'sessionClosed' being volatile ie bc multiple threads can read/write it and in this particular case a limited scope optimizatiion compiler might decide to otherwise optimize away the 'if' block in this loop
>>  
>> https://github.com/openjdk/jdk/pull/16794/commits/6805f72d4b7f33d5bfa0ae5742122d377345c6e2#diff-3ba5df79cdd3da36b21bf29b4e8de462dd61e6a21dfe0816e4d84c18bbfb76b2R927
>> 
>> In practice tho I dont think modern compilers would do that but since there is no real cost of using volatile in this case as there is nothing practical to gain by optimizing around it, its best to explicitly tell the compiler to not mess with it at all than to rely on any assumption that it just would not as those optimizations are implementation dependent.
>
>> hey, a fellow Sun comrade! (i still have my Sun badge too) :)
> 
> :handshake: 
> 
>> I dont think modern compilers would do that
> 
> I would argue that no standard-compliant compiler would do that.
> 
>  `volatile` is very hard to summarize without loosing accuracy (see WG21 6.7.3/7 and Annex C) , but they are about hardware interrupts/signal handlers and the like that can "suddenly" change the value ("modified in ways unknown to the implementation"), not about multithreading and certainly neither replace nor enhance synchronization. 
> 
>> compiler might decide to otherwise optimize away the 'if' block in this loop
> 
> That would be a (huge) bug in the compiler. The variable is global, the rules of the abstract machine allow for the value to change outside of the loop and outside of the containing function.
> 
> FWIW, `sessionClosed` also doesn't have to be volatile.
> 
>> there is no real cost of using volatile in this case
> 
> I agree that this is hardly the hottest spot of the entire affair of taking a screenshot, but the cost is in readability. `volatile` has a very special meaning and here it is used for a purpose that does not match this meaning. This is at least confusing. Just like you wouldn't use _Atomic in a single-threaded application even though it might cost nothing overall, you shouldn't use volatile simply because it's cost-less in terms of performance.

@mkartashev WG21 is C++ standard and this is not. Here is what GNU docs say, specifically, quote "The standards encourage compilers to refrain from optimizations concerning accesses to volatile objects. The C standard leaves it implementation defined as to what constitutes a volatile access.".

Regarding readability I would actually argue the other way. If I see something declared as volatile I would more likely to take care to pay special attention to it which is beneficial for the code in question, especially given those synchronization issues around it. 

Either way, I dont believe there is any practical reason to waste time and have an argument about this, so if you feel strongly about it (I don't) I can drop 'volatile' from both variables if that helps.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16794#discussion_r1412160104