Does this look like a race?

[David Holmes]

On 13/08/2014 1:01 AM, Andrew Haley wrote:
> (Please forgive me if this should go to the AWT list or somesuch.  It
> seems to me like this is not really a graphics-related issue, but one
> of Java concurrency.)
>
> This is in JDK9, sun.font.GlyphList.  There is a non-volatile boolean
> inUse, and it is not always written in a synchronized block.  It is
> used to allow exclusive access to a singleton instance.
>
> It seems to me that, at a minimum, inUse should be volatile, or e.g.
> strikelist might be overwritten to null after some other thread has
> started using this GlyphList.  Do you agree?

Yes, either inUse needs to be volatile and used in a way that ensures 
the correct happens-before relationships, or dispose() needs to do all 
modifications to the shared instance inside a synchronized block  and 
clear inUse in that block.

David

> Thanks,
> Andrew.
>
>
>      /* This scheme creates a singleton GlyphList which is checked out
>       * for use. Callers who find its checked out create one that after use
>       * is discarded. This means that in a MT-rendering environment,
>       * there's no need to synchronise except for that one instance.
>       * Fewer threads will then need to synchronise, perhaps helping
>       * throughput on a MP system. If for some reason the reusable
>       * GlyphList is checked out for a long time (or never returned?) then
>       * we would end up always creating new ones. That situation should not
>       * occur and if it did, it would just lead to some extra garbage being
>       * created.
>      private static GlyphList reusableGL = new GlyphList();
>      private static boolean inUse;
>
>      ...
>
>      public static GlyphList getInstance() {
>          /* The following heuristic is that if the reusable instance is
>           * in use, it probably still will be in a micro-second, so avoid
>           * synchronising on the class and just allocate a new instance.
>           * The cost is one extra boolean test for the normal case, and some
>           * small number of cases where we allocate an extra object when
>           * in fact the reusable one would be freed very soon.
>           */
>          if (inUse) {
>              return new GlyphList();
>          } else {
>              synchronized(GlyphList.class) {
>                  if (inUse) {
>                      return new GlyphList();
>                  } else {
>                      inUse = true;
>                      return reusableGL;
>                  }
>              }
>          }
>      }
>
>      ...
>
>      /* There's a reference equality test overhead here, but it allows us
>       * to avoid synchronizing for GL's that will just be GC'd. This
>       * helps MP throughput.
>       */
>      public void dispose() {
>          if (this == reusableGL) {
>              if (graybits != null && graybits.length > MAXGRAYLENGTH) {
>                  graybits = null;
>              }
>              usePositions = false;
>              strikelist = null; // remove reference to the strike list
>              inUse = false;
>          }
>      }
>