Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8094711

[Canvas] Garbled paths when drawing paths in Canvas'es from multiple Thread's because of use of global variables in GraphicsContext

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: P2
    • Resolution: Fixed
    • Affects Version/s: fx2.1, 7u6, 8, 8u20
    • Fix Version/s: 8u20
    • Component/s: javafx
    • Labels:
    • Environment:

      Mac OS X 10.9 and Java 8 GA release

      Description

      Garbled paths when drawing paths in Canvas'es from multiple Thread's because of use of global temporary variables.

      Issue is that paths get garbled when several Thread's are rendering to Canvas'es at the same time. The issue is caused by global temporary variables in GraphicsContext. For a more detailed explanation see the code below that demonstrates the bug (just create a template NetBeans / Eclipse JavaFX project and run the program):

      import java.util.ArrayList;
      import java.util.Arrays;
      import java.util.List;

      import javafx.application.Application;
      import javafx.application.Platform;
      import javafx.scene.Group;
      import javafx.scene.Node;
      import javafx.scene.Parent;
      import javafx.scene.Scene;
      import javafx.scene.canvas.Canvas;
      import javafx.scene.canvas.GraphicsContext;
      import javafx.scene.control.CheckBox;
      import javafx.scene.control.ScrollPane;
      import javafx.scene.layout.Priority;
      import javafx.scene.layout.VBox;
      import javafx.stage.Stage;

      /**
       * A demo of a concurrency bug in the path creation methods of the
       * <code>GraphicsContext</code> class.
       *
       * The demo creates a window having a checkbox at the top controlling
       * the behavior of the demo and a scrollpane below the checkbox displaying
       * a number of "startburst" tiles in a regular grid.
       *
       *
       * The tiles are each individual <code>Canvas</code>'es created concurrently
       * by several <code>Thread</code> and which are then added to a <code>Group</code>
       * on the JavaFX application <code>Thread</code>.
       *
       *
       * Initially the demo is set up to show the concurrency bug:
       *
       * The "starburst" lines of the tiles are shown in more or less random
       * positions inside the tile. This is caused by overlapping access to
       * "static" temporary variables used by the path creating methods of
       * the <code>GraphicsContext</code> class. See below for a more detailed
       * explanation of the issue.
       *
       *
       * The demo can be run in two modes controlled by the "Creating tiles locked"
       * checkbox:
       *
       * Checking this enables a workaround for the concurrency bug (see description
       * of workaround below)
       *
       * Unchecking this, disables the workaround and demo's the effects of the
       * concurrency bug (which essentially randomizes the results of the path
       * creation methods)
       */

      public class GraphicsContextExample extends Application {
        final Object canvasPathMethodsLock = new Object();
        
        /**
         * Creates a "tile" with "starburst" lines at i,j. The creation is
         * possibly done under locking using the <code>createTileLock</code>
         * lock.
         *
         * This is controlled by the "Creating tiles locked" checkbox
         * in the window of this bug demo
         */
        private Node createTile(final boolean createTilesLocked, final int i, final int j) {
          final int tileSize = 100;
          final int tileInset = 10;
          
          final int tileLineCount = 20;
          final int tileLinesRadius = (tileSize - (tileInset * 2)) / 2;

          final Canvas tile = new Canvas(tileSize, tileSize);
          tile.setManaged(false);
          
          final GraphicsContext gc = tile.getGraphicsContext2D();
          
          for (int tileLineNo = 0; tileLineNo < tileLineCount; tileLineNo++) {
            gc.save();
            
            gc.translate(tileSize / 2, tileSize / 2);
            gc.rotate((360.0 / tileLineCount) * tileLineNo);
        
            /**
             * IMPORTANT:
             *
             * This is the interesting part of this bug demo, here the calling
             * <code>Thread</code> is either creating the path for it's <code>Canvas</code>
             * concurrently with other <code>Thread</code>'s creating path's for their
             * own <code>Canvas</code>'es OR the calling <code>Thread</code> is creating
             * the path in a serialized manner so that only ONE <code>Thread</code> is
             * allowed to create the path for it's <code>Canvas</code> at a time.
             *
             * Essentially the code below either allows concurrent access to the
             * <code>GraphicsContext</code> methods for creating paths or serializes
             * access to the methods for creating paths.
             *
             * The reason that serializing access to the path creation methods of
             * <code>GraphicsContext</code> makes a difference is that these path methods
             * use "static" global temporary variables during various calculations, obviously
             * for performance gaining purposes. E.g. look in the <code>GraphicsContext</code>
             * code for the following snippet:
             *
             * private static float coords[] = new float[6];
             *
             * and search for references to this global variable. This one is probably the
             * worst offender, there are other "static" global variables used in the same
             * manner that affect other <code>GraphicsContext</code> methods: just search
             * for "static" global variables in the <code>GraphicsContext</code> code.
             *
             * This, unfortunately, conflicts with concurrent use of these methods, which
             * is explicitly allowed by the <code>GraphicsContext</code> class (and which
             * is one of it's attractions: allow concurrent rendering outside the JavaFX
             * application <code><Thread/code> as long as the <code>Canvas</code> has not
             * been added to the <code>Node</code> hierarchy and is only accessed by one
             * <code>Thread</code> during the rendering).
             *
             * The workaround for the "static" temporary variable use is to protect access
             * to these variables using a lock to serialize access to all <code>GraphicsContext</code>
             * methods using these variables. Unfortunately, this sort of defeats the
             * concurrency promise of the <code>GraphicsContext</code> class and is also
             * difficult to enforce in large projects, especially if using third party
             * libraries for which the source code is not readily available.
             *
             * The fix is simple: make the temporary "static" variables into <code>GraphicsContext</code>
             * members (just by removing the "static" qualifier). This ensures that the "one thread use"
             * promise of the <code>GraphicsContext</code> class is still valid AND also ensures that
             * most of the performance optimizing effects of using the previous "static" temporary
             * variables are still present.
             */
            
            if (createTilesLocked) {
              synchronized (canvasPathMethodsLock) {
                gc.beginPath();
                
                gc.moveTo(0, 0);
                gc.lineTo(0, tileLinesRadius);

                gc.closePath();

                gc.stroke();
              }
            } else {
              gc.beginPath();
              
              gc.moveTo(0, 0);
              gc.lineTo(0, tileLinesRadius);

              gc.closePath();

              gc.stroke();
            }

            /**
             * END OF IMPORTANT SECTION.
             *
             * See comment above.
             */
            
            gc.restore();
          }
            
          tile.setLayoutX(i * tileSize);
          tile.setLayoutY(j * tileSize);
          
          return tile;
        }

        /**
         * Creates the "tile"'s that shows the "starburst" lines. The "tile"'s
         * are created concurrently by the "parallel" stream, see below
         */
        private Node createTiles(final boolean createTilesLocked) {
          final int noOfTilesHorizontally = 50;
          final int noOfTilesVertically = 30;
          
          final List<List<Integer>> tiles = new ArrayList<List<Integer>>();
          
          for (int i = 0; i < noOfTilesHorizontally; i++) {
            for (int j = 0; j < noOfTilesVertically; j++) {
              tiles.add(Arrays.asList(i, j));
            }
          }

          final Group tileGroup = new Group();
          tileGroup.setManaged(false);
          
          // I just *had* to use at least one new Java 8 feature: Streams... ;-)
          tiles
            .stream()
            .parallel()
            .map((tileIJ) -> createTile(createTilesLocked, tileIJ.get(0), tileIJ.get(1)))
            .parallel()
            .forEach((tile) -> {
              Platform.runLater(() -> {
                tileGroup.getChildren().add(tile);
              });
            });
          
          return tileGroup;
        }

        /**
         * Creates the contents <code>Node</code> for the Canvas bug demo
         * stage/scene. The contents is a checkbox that controls the bug demo
         * and a scrollpane that holds the "tile"'s created by <code>createTiles</code>
         */
        private Parent createGraphicsContextExample() {
          final boolean tilesInitiallyCreatedLocked = false;
          
          final ScrollPane scrollPane = new ScrollPane(createTiles(tilesInitiallyCreatedLocked));
          VBox.setVgrow(scrollPane, Priority.ALWAYS);
          
          final CheckBox checkbox = new CheckBox("Creating tiles locked");
          checkbox.setStyle("-fx-background-color:transparent; -fx-border-width:5; -fx-border-color:transparent");
          checkbox.setSelected(tilesInitiallyCreatedLocked);
          
          checkbox.setOnAction((e) -> {
            scrollPane.setContent(createTiles(checkbox.isSelected()));
          });
          
          return new VBox(checkbox, scrollPane);
        }
        
        /**
         * Setup the <code>Stage</code> and show this to the developer, nicely
         * centered and all :) Check or uncheck to see the effects of creating
         * and drawing path's <code>Canvas</code>'es concurrently or one thread
         * at a time
         */
        @Override
        public void start(final Stage primaryStage) throws Exception {
          primaryStage.setTitle("GraphicsContext concurrency bug example");
          primaryStage.setScene(new Scene(createGraphicsContextExample()));
          
          primaryStage.setWidth(900);
          primaryStage.setHeight(500);
          
          primaryStage.show();
          primaryStage.centerOnScreen();
        }
        
        /**
         * Start of the world...
         */
        public static void main(String[] args) {
          Application.launch(args);
        }
      }

        Attachments

          Activity

            People

            Assignee:
            flar Jim Graham
            Reporter:
            cclaessonjfx Christa Claesson (Inactive)
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:
              Imported: