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

[Sergey Bylokhov]

On 25.07.2017 2:48, Jim Graham wrote:
> This same thing is done in other places in a simpler way without 
> creating an inner class simply by having createGE() do the assignment 
> and test for null, as in:

I guess a solution which you mentions is a variant of "Double-checked 
locking".

> => after:
> 
>      public static GraphicsEnvironment getLocalGraphicsEnvironment() {
>          if (c== null) {
>              createGE();
>          }
> 
>          return localEnv;
>      }
> 
>      private static synchronized GraphicsEnvironment createGE() {
>          if (localEnv != null) return;
>          ...
>          localEnv = ge;
>      }
>

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).


> OR => alternate after:
> 
>      public static synchronized GraphicsEnvironment 
> getLocalGraphicsEnvironment() {
>          if (localEnv == null) {
>              synchronized (GraphicsEnvironment.class) {
>                  if (localEnv == null) {
>                      localEnv = createGE();
>                  }
>              }
>          }
> 
>          return localEnv;
>      }
> 
>      // And no changes to createGE()
> 

In this variant the field also should be volatile. I guess it was a typo 
that the getLocalGraphicsEnvironment still "synchronized".

> Note that there is a test for null both in getLGE() and also again in 
> createGE() because the second one is inside a synchronized block and 
> required to prevent multiple instances.  The first one in getLGE() 
> avoids having to synchronize in the first place for the common case.

Note that in case of holder - after the first call there was not any 
synchronization(like in case of "synchronized" and "volatile").

Here some additional information which I used:
https://shipilev.net/blog/2014/safe-public-construction/

> 
>              ...jim
> 
> On 7/24/17 5:09 PM, Sergey Bylokhov wrote:
>> Hello,
>> Please review the fix for jdk10.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8185093
>> Webrev can be found at: 
>> http://cr.openjdk.java.net/~serb/8185093/webrev.00
>> jmh test: http://cr.openjdk.java.net/~serb/8185093/LocalGEproblem.java
>>
>> While working on some other bug I found a performance issue in our 
>> java2d pipeline in multi-threaded environment. We have a "hot spot" in 
>> the "GraphicsEnvironment.getLocalGraphicsEnvironment()". This method 
>> is executed every time the graphics object is created, for example 
>> BufferedImage.java:
>>
>>      public Graphics2D createGraphics() {
>>          GraphicsEnvironment env =
>>              GraphicsEnvironment.getLocalGraphicsEnvironment();
>>          return env.createGraphics(this);
>>      }
>>
>> So even if the application will draw to a different Images it will be 
>> blocked for some time in this method.
>> I created a jmh test case which shows that implementing this method 
>> via holder will speedup the next code:
>>        Graphics2D graphics = bi.createGraphics();
>>        graphics.drawLine(0, 0, 100, 100);
>>
>>   4 Threads:
>>   - Before the fix:  8922 ops/ms
>>   - After the fix :  9442 ops/ms
>>   8 Threads:
>>   - Before the fix:  4511 ops/ms
>>   - After the fix : 11899 ops/ms
>>
>> The main issue which I would like to clarify is it possible to remove 
>> synchronize keyword from the getLocalGraphicsEnvironment(), possibly 
>> with updating a specification? We have similar issues in other parts 
>> of code which I would like to update later after this one.
>>


-- 
Best regards, Sergey.