[OpenJDK 2D-Dev] [10] Review Request: 8185093 Expensive multi-core choke point when any graphics objects are created

[Jim Graham]

Hi Sergey,

On 7/25/17 12:53 PM, Sergey Bylokhov wrote:
> In this variant the field should be volatile, and this will introduce some synchronization. Otherwise it will be 
> possible to read non-null value in localEnv while the constructor of GraphicsEnvironment was not completed on some other 
> thread(but the field was assigned already).

Even though the assignment is the last statement in the method?

In looking through the article that you sent out (only briefly) it looks like the consideration is that the 
initialization method run in one thread could reorder the instructions at run time so that the field is assigned before 
the initialization steps are done, even if the code is written with a different specific ordering.

I'm guessing that the Java Memory Model ensures that membars are placed to protect static field initialization from 
similar dangers?

If that is the case then I think we have a few places where we do this "manual conditional synchronization" that should 
probably be investigated.  If I remember correctly, though, they may use the following paradigm which might not suffer 
from that issue:

Does it help if a local variable is used, as in:

GE env = this.localEnv
if (env == null) {
     synchronized (class) {
         if (this.localEnv == null) {
             localEnv = createGE();
         }
         env = this.localEnv;
     }
}
return env;

This ensures that the second read from the field is done in the synchronized block for the case that it was originally 
found to be null, though this might not help if the line "localEnv = createGE()" can reorder code from inside the call 
to createGE() to code outside of it.  However those dangling reordered stores should be resolved before it leaves the 
synchronized block, no?

In any case, I can see that the code used in the webrev will eventually have no dead code at all in the final compiled 
version of the method whereas this version would minimally at least have a null check that must be executed even though 
it would be vestigial once the initialization was done...

+1 for the inner class version - it's the most straightforward way to do this optimally...

				...jim