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

[Sergey Bylokhov]

On 25.07.2017 15:03, Jim Graham wrote:
> 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?

The assignment include these operations:
  - Allocate memory
  - Initialize the object
  - Assign the object to the field.

These operation may be reordered, so some other thread can see non-null 
reference in the field while initialization was not complete.

> 
> 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?

The key feature here is initialization of the class.
The next line will be blocked while the LocalGE class will not be 
initialized:
     return LocalGE.INSTANCE;
And initialization will start only when this code will run for the first 
time.

> 
> 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?

I think that it is possible that one thread will do:
  - create an object
  - assign the object to the localEnv.
  - Initialize the object.
And the second thread will do:
  - Read non-null value from the this.localEnv to the env
  - Check env to null
  - Return non-initialized object.

> 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...
-- 
Best regards, Sergey.