GUACAMOLE-377: Reduce thread contention by flushing simple operations…

… early.

src/libguac/display-plan.c

@@ -20,9 +20,12 @@
 #include "display-plan.h"
 #include "display-priv.h"
 #include "guacamole/assert.h"
+#include "guacamole/client.h"
 #include "guacamole/display.h"
 #include "guacamole/fifo.h"
 #include "guacamole/mem.h"
+#include "guacamole/protocol.h"
+#include "guacamole/socket.h"
 #include "guacamole/timestamp.h"
 
 #include <string.h>
@@ -365,10 +368,59 @@ void guac_display_plan_free(guac_display_plan* plan) {
 void guac_display_plan_apply(guac_display_plan* plan) {
 
     guac_display* display = plan->display;
+    guac_client* client = display->client;
     guac_display_plan_operation* op = plan->ops;
 
+    /* Immediately send instructions for all updates that do not involve
+     * significant processing (do not involve encoding anything). This allows
+     * us to use the worker threads solely for encoding, reducing contention
+     * between the threads. */
     for (int i = 0; i < plan->length; i++) {
-        guac_fifo_enqueue(&display->ops, op++);
+
+        guac_display_layer* display_layer = op->layer;
+        switch (op->type) {
+
+            case GUAC_DISPLAY_PLAN_OPERATION_COPY:
+                guac_protocol_send_copy(client->socket, op->src.layer_rect.layer,
+                        op->src.layer_rect.rect.left, op->src.layer_rect.rect.top,
+                        guac_rect_width(&op->src.layer_rect.rect), guac_rect_height(&op->src.layer_rect.rect),
+                        GUAC_COMP_OVER, display_layer->layer, op->dest.left, op->dest.top);
+                break;
+
+            case GUAC_DISPLAY_PLAN_OPERATION_RECT:
+
+                guac_protocol_send_rect(client->socket, display_layer->layer,
+                        op->dest.left, op->dest.top, guac_rect_width(&op->dest), guac_rect_height(&op->dest));
+
+                int alpha = (op->src.color & 0xFF000000) >> 24;
+                int red   = (op->src.color & 0x00FF0000) >> 16;
+                int green = (op->src.color & 0x0000FF00) >> 8;
+                int blue  = (op->src.color & 0x000000FF);
+
+                /* Clear before drawing if layer is not opaque (transparency
+                 * will not be copied correctly otherwise) */
+                if (!display_layer->opaque)
+                    guac_protocol_send_cfill(client->socket, GUAC_COMP_ROUT, display_layer->layer,
+                            0x00, 0x00, 0x00, 0xFF);
+
+                guac_protocol_send_cfill(client->socket, GUAC_COMP_OVER, display_layer->layer,
+                        red, green, blue, alpha);
+
+                break;
+
+            /* Simply ignore and drop NOP */
+            case GUAC_DISPLAY_PLAN_OPERATION_NOP:
+                break;
+
+            /* All other operations should be handled by the workers */
+            default:
+                guac_fifo_enqueue(&display->ops, op);
+                break;
+
+        }
+
+        op++;
+
     }
 
 }

src/libguac/display-priv.h

@@ -92,7 +92,6 @@
  * 2) last_frame.lock
  * 3) ops
  * 4) render_state
- * 5) op_path_lock
  *
  * Acquiring these locks in any other order risks deadlock. Don't do it.
  */
@@ -667,15 +666,6 @@ struct guac_display {
      */
     guac_fifo ops;
 
-    /**
-     * Lock which ensures instructions that make use of a layer's current path
-     * (such as "rect" and "cfill") do not get inadvertently interleaved.
-     * Interleaving of path instructions can result in those paths not matching
-     * the expectatiosn of subsequent instructions, causing graphical
-     * artifacts.
-     */
-    pthread_mutex_t op_path_lock;
-
     /**
      * Storage for any items within the ops fifo.
      */

src/libguac/display-worker.c

@@ -104,16 +104,12 @@ static void guac_display_layer_clear_non_opaque(guac_display_layer* display_laye
      * being non-opaque */
     if (!display_layer->opaque) {
 
-        pthread_mutex_lock(&display->op_path_lock);
-
         guac_protocol_send_rect(socket, layer, dirty->left, dirty->top,
                 guac_rect_width(dirty), guac_rect_height(dirty));
 
         guac_protocol_send_cfill(socket, GUAC_COMP_ROUT, layer,
                 0x00, 0x00, 0x00, 0xFF);
 
-        pthread_mutex_unlock(&display->op_path_lock);
-
     }
 
 }
@@ -371,36 +367,14 @@ void* guac_display_worker_thread(void* data) {
                 break;
 
             case GUAC_DISPLAY_PLAN_OPERATION_COPY:
-                guac_protocol_send_copy(client->socket, op.src.layer_rect.layer,
-                        op.src.layer_rect.rect.left, op.src.layer_rect.rect.top,
-                        guac_rect_width(&op.src.layer_rect.rect), guac_rect_height(&op.src.layer_rect.rect),
-                        GUAC_COMP_OVER, display_layer->layer, op.dest.left, op.dest.top);
-                break;
-
             case GUAC_DISPLAY_PLAN_OPERATION_RECT:
-
-                pthread_mutex_lock(&display->op_path_lock);
-                guac_protocol_send_rect(client->socket, display_layer->layer,
-                        op.dest.left, op.dest.top, guac_rect_width(&op.dest), guac_rect_height(&op.dest));
-
-                int alpha = (op.src.color & 0xFF000000) >> 24;
-                int red   = (op.src.color & 0x00FF0000) >> 16;
-                int green = (op.src.color & 0x0000FF00) >> 8;
-                int blue  = (op.src.color & 0x000000FF);
-
-                /* Clear before drawing if layer is not opaque (transparency
-                 * will not be copied correctly otherwise) */
-                if (!display_layer->opaque)
-                    guac_protocol_send_cfill(client->socket, GUAC_COMP_ROUT, display_layer->layer,
-                            0x00, 0x00, 0x00, 0xFF);
-
-                guac_protocol_send_cfill(client->socket, GUAC_COMP_OVER, display_layer->layer,
-                        red, green, blue, alpha);
-
-                pthread_mutex_unlock(&display->op_path_lock);
-                break;
-
             case GUAC_DISPLAY_PLAN_OPERATION_NOP:
+                guac_client_log(client, GUAC_LOG_DEBUG, "Operation type %i "
+                        "should NOT be present in the set of operations given "
+                        "to guac_display worker thread. All operations except "
+                        "IMG and END_FRAME are handled during the initial, "
+                        "single-threaded flush step. This is likely a bug.",
+                        op.type);
                 break;
 
             case GUAC_DISPLAY_PLAN_END_FRAME:

src/libguac/display.c

@@ -137,9 +137,6 @@ guac_display* guac_display_alloc(guac_client* client) {
     guac_flag_init(&display->render_state);
     guac_flag_set(&display->render_state, GUAC_DISPLAY_RENDER_STATE_FRAME_NOT_IN_PROGRESS);
 
-    /* Init lock specific to the GUAC_DISPLAY_PLAN_OPERATION_RECT operation */
-    pthread_mutex_init(&display->op_path_lock, NULL);
-
     int cpu_count = guac_display_nproc();
     if (cpu_count <= 0) {
         guac_client_log(client, GUAC_LOG_WARNING, "Number of available "
@@ -176,7 +173,6 @@ void guac_display_free(guac_display* display) {
         pthread_join(display->worker_threads[i], NULL);
 
     /* All locks, FIFOs, etc. are now unused and can be safely destroyed */
-    pthread_mutex_destroy(&display->op_path_lock);
     guac_flag_destroy(&display->render_state);
     guac_fifo_destroy(&display->ops);
     guac_rwlock_destroy(&display->last_frame.lock);