My preference is just to use volatile. AtomicReference is basically some fancy wrapper on top of it that _in this case_ wouldn't add value. I understand volatile.
~ David Smiley Apache Lucene/Solr Search Developer http://www.linkedin.com/in/davidwsmiley On Wed, Mar 17, 2021 at 10:52 AM Michael Gibney <[email protected]> wrote: > It looks like since jdk 1.5, making the array reference volatile will > prevent re-ordering array element writes (with respect to the volatile > array reference write): > > https://web.archive.org/web/20200923063441/https://www.ibm.com/developerworks/library/j-jtp03304/index.html > > So the code as initially written (sorry!) was not thread-safe, and the > solution initially proposed ("The fix would be declaring the array > volatile") would indeed fix the problem? i.e.: > > private volatile int[] cachedOrdIdxMap; > > private int[] getOrdIdxMap(...) { > final int[] local = cachedOrdIdxMap; > if (local != null) { > return local; > } else { > return cachedOrdIdxMap = compute(...); // idempotent (same result for > same input) > } > } > > On Wed, Mar 17, 2021 at 8:11 AM Uwe Schindler <[email protected]> wrote: > > > Hi, > > > > I agree with Dawid to make it developer friendly. Most > synchronized-blocks > > are rmeoved by Hotspot anyways, so better safe than sorry! > > David: I missed in your original mail that you were afraid of changes > > inside the array not being visible. You can prevent that with fences, but > > all of that is hard to understand. > > > > IMHO, I would use an AtomicReference: > > > > final AtomicReference<V> arr = new AtomicReference<>(); > > public V getLazy() { > > V result = arr.get(); > > if (result == null) { > > result = compute(...); > > if (!arr.compareAndSet(null, result)) { > > return arr.get(); > > } > > } > > return result; > > } > > > > ----- > > Uwe Schindler > > Achterdiek 19, D-28357 Bremen > > https://www.thetaphi.de > > eMail: [email protected] > > > > > -----Original Message----- > > > From: Dawid Weiss <[email protected]> > > > Sent: Wednesday, March 17, 2021 9:01 AM > > > To: [email protected] > > > Subject: Re: Thread-safety without volatile for an immutable array > > > > > > Unless this code is used millions of millions of times I would just > use a > > > code pattern that is understood by any developer - senior and > junior... A > > > volatile or even initialization under a plain monitor. Those fancy > memory > > > guards are great for low-level data structures but they are really hard > > to > > > understand (even for folks who have been using Java for long). > > > > > > D. > > > > > > On Wed, Mar 17, 2021 at 4:22 AM David Smiley <[email protected]> > wrote: > > > > > > > Moreover, AFAICT, in SharedSecrets, those fields were all immutable > -- > > no > > > > state. I spot checked a couple and saw no fields at all. Even final > > > > fields would be fine because of "effectively final" classes (e.g. > > String). > > > > But an int array is another matter; no? > > > > > > > > ~ David Smiley > > > > Apache Lucene/Solr Search Developer > > > > http://www.linkedin.com/in/davidwsmiley > > > > > > > > > > > > On Tue, Mar 16, 2021 at 6:30 PM Uwe Schindler <[email protected]> > wrote: > > > > > > > > > Hi, > > > > > > > > > > it is theoretically possible (but unlikely). There was a discussion > > on > > > > the > > > > > mailinglist of openjdk, because SharedSecrets had the same problem. > > > > > > > > > > There's a workaround for that: Use a local variable (copy value to > a > > > > local > > > > > variable, then test for null and run the initialization)! See the > > fix for > > > > > SharedSecrets in the JDK: > > > > > https://github.com/openjdk/jdk/commit/85bac8c4 > > > > > The trick is here: The order must be ensured in the actual thread, > > so the > > > > > local variable is exactly what you expect and cannot suddenly > change > > to > > > > > null by reordering. So when you copy the global to a local and > return > > > > that > > > > > one at end, you know that it's as you expect. > > > > > > > > > > You could also add a fence, so the order is preserved, I think for > > that > > > > > the following is fine (put it before the write inside the if > clause): > > > > > < > > > > > > > > > > > > > > > https://docs.oracle.com/javase/9/docs/api/java/lang/invoke/VarHandle.html#a > > > cquireFence-- > > > > > > > > > > > > > > > > I'd go with what the JDK developers did. > > > > > > > > > > Uwe > > > > > > > > > > ----- > > > > > Uwe Schindler > > > > > Achterdiek 19, D-28357 Bremen > > > > > https://www.thetaphi.de > > > > > eMail: [email protected] > > > > > > > > > > > -----Original Message----- > > > > > > From: David Smiley <[email protected]> > > > > > > Sent: Tuesday, March 16, 2021 8:56 PM > > > > > > To: [email protected] > > > > > > Cc: Michael Gibney <[email protected]> > > > > > > Subject: Thread-safety without volatile for an immutable array > > > > > > > > > > > > I'm code reviewing something over here: > > > > > > > > > > > > https://github.com/apache/solr/pull/2/files#diff- > > > > > > > > > aa2f75bf39a49e1fcd52aacff89da3ea93f622803eb5996d5edb5e04070a50b1R6 > > > > > > 58 > > > > > > > > > > > > In a nutshell, it looks like this: > > > > > > > > > > > > private int[] cachedOrdIdxMap; > > > > > > > > > > > > private int[] getOrdIdxMap(...) { > > > > > > if (cachedOrdIdxMap == null) { > > > > > > cachedOrdIdxMap = compute(...); // idempotent (same result > > for > > > > same > > > > > > input) > > > > > > } > > > > > > return cachedOrdIdxMap; > > > > > > } > > > > > > > > > > > > I suspect this is not thread-safe because the value is not > > "published" > > > > > > safely, and thus it might be possible for a reader to see a > > > > > > non-null cachedOrdIdxMap yet it might not be populated yet > because > > the > > > > > > machine is allowed to re-order reads and writes in the absence of > > any > > > > > > synchronization controls. The fix would be declaring the array > > > > > volatile, I > > > > > > think. > > > > > > > > > > > > Thoughts? > > > > > > > > > > > > ~ David Smiley > > > > > > Apache Lucene/Solr Search Developer > > > > > > http://www.linkedin.com/in/davidwsmiley > > > > > > > > > > > > > > > > > > >
