Does this look like a race?

[Stanimir Simeonoff]

Hi,

The real problem would be fields strikelist, graybits and the like not
inUse per se. They are not volatile either and there might not be proper
happens before edges, so they could easily be updated out of order. For
instance getGrayBits may throw a NPE.

IMO, inUse is overall a poor choice to implement the shared singleton, esp
since introduces a synchronized block that can be easily avoided. inUse and
the sync block can be replaced by an AtomicReference +CAS on get.
Something like that:
private static final AtomicReference<GlyphList > shared=new
AtomicReference<GlyphList >(reusableGL);
public static GlyphList getInstance() {
GlyphList result=shared.get();
return result!=null && shared.compareAndSet(result, null)?result:new
GlyphList();
}

public void dispose(){
if (this==reusableGL){
....
strikelist = null;
shared.set(this);//or if (!shared.compareAndSet(null, this)) throw new
AssertionError();
}
}

Regards
Stanimir



On Tue, Aug 12, 2014 at 6:01 PM, Andrew Haley <aph at redhat.com> 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?
>
> 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;
>         }
>     }
>