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.

Reply via email to