On Fri, 1 Dec 2023 13:44:21 GMT, Maxim Kartashev <mkartas...@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
