render_multi works a bit more efficiently now
authorTero Marttila <terom@fixme.fi>
Sun, 08 Jun 2008 23:10:36 +0300
changeset 16 50995bbe442a
parent 15 e7f0697814dc
child 17 8e8b56b0e0f5
render_multi works a bit more efficiently now

committer: Tero Marttila <terom@fixme.fi>
render_multi.c
render_multi.h
render_remote.c
render_remote.h
web_main.c
--- a/render_multi.c	Sun Jun 08 21:03:23 2008 +0300
+++ b/render_multi.c	Sun Jun 08 23:10:36 2008 +0300
@@ -1,4 +1,6 @@
 #include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
 #include <assert.h>
 
 #include "common.h"
@@ -8,22 +10,42 @@
 #include "remote_node.h"
 #include "render_png.h"
 
+/*
+// the states we can go through
+enum render_multi_state {
+    STATE_INIT,                 // the render_multi is in the process of being initialized
+    STATE_SENDING,              // we're still waiting for some of the requests to be sent
+    STATE_RENDER,               // we're handling data now!
+    STATE_DATA_DONE,            // we're finished with all the data
+    STATE_PNG_DONE,             // PNG data's done
+    STATE_FAILED,               // we failed
+};
+
+enum render_multi_sub_state {
+    STATE_INIT,                 // the render_multi_sub is in the process of being initialized
+    STATE_SENDING,              // we're waiting for the requests to be sent
+    STATE_FILL_ROW,             // we're filling the row with data
+    STATE_ROW_FULL,             // our row is full
+    STATE_DATA_DONE,            // we're finished with all the data
+    STATE_PNG_DONE,             // PNG data's done
+    STATE_FAILED,               // we failed
+
+};
+*/
+
 struct render_multi {
     // these are used as arguments to render_remote
     struct render_multi_sub {
         // our offset in the list
         int index;
 
-        // the remote_render_ctx
-        struct render_remote *remote_render;
+        // the render_remote_ctx
+        struct render_remote *render_remote;
         
         // a pointer to ourself
         struct render_multi *self;
 
-        // if the render connection is marked as done without us having read all the data, move it from render_remote's buffers into here
-        struct evbuffer *in_buf;
-
-        // _render_sent called for this?
+        // _render_multi_sent called for this?
         int render_sent;
 
         // our offset into the row, static
@@ -35,19 +57,32 @@
         // how many bytes we have already written into the current row
         size_t col;
 
-        // is this render op done?
+        // _render_multi_done called for this?
         int render_done;
+
     } remote_renders[RENDER_MULTI_NODES_MAX];
     
     // how many remote_renders we have
     int remote_render_count;
 
+    // is this still alive?
+    int alive;
+
+    // how many remote renders have been succesfully cb_sent?
+    int renders_sent;
+
+    // how many of the renders are done?
+    int renders_done;
+
     // have we called cb_sent?
     int have_sent;
 
     // the png thing
     struct render_png *png_info;
 
+    // has render_png_done returned?
+    int png_done;
+
     // our pixel data row
     unsigned char *rowbuf;
 
@@ -69,6 +104,8 @@
 // prototypes
 static void _render_multi_do_free (struct render_multi *ctx);
 static void _render_multi_do_sent (struct render_multi *ctx);
+static void _render_multi_do_png_data (struct render_multi *ctx);
+static void _render_multi_do_png_done (struct render_multi *ctx);
 static void _render_multi_do_done (struct render_multi *ctx);
 static void _render_multi_do_fail (struct render_multi *ctx, int flags);
 
@@ -76,17 +113,8 @@
  * Actually free the request. Should be de-initialized (either _render_multi_error, or _render_done when this is called
  */
 static void _render_multi_do_free (struct render_multi *ctx) {
-    if (!ctx)
-        return;
+    assert(ctx && ctx->alive == 0);
     
-    int i;
-    for (i = 0; i < ctx->remote_render_count; i++) {
-        if (ctx->remote_renders[i].in_buf) {
-            evbuffer_free(ctx->remote_renders[i].in_buf);
-            ctx->remote_renders[i].in_buf = NULL;
-        }
-    }
-
     if (ctx->rowbuf) {
         free(ctx->rowbuf);
         ctx->rowbuf = NULL;
@@ -119,48 +147,75 @@
     
     // we're going to always have the PNG header data buffered at this point, so give that to the user right away
     assert(evbuffer_get_length(ctx->out_buf) > 0);
+    _render_multi_do_png_data(ctx);
+}
 
-    ctx->cb_data(ctx->out_buf, ctx->cb_arg);
+// possibly call cb_data, and if the renders are all done and the buffer is empty, cb_done
+static void _render_multi_do_png_data (struct render_multi *ctx) {
+    // at first we have to wait until we've called cb_sent
+    if (!ctx->have_sent)
+        return;
 
-    // ok
+    // got any PNG data in there?
+    if (evbuffer_get_length(ctx->out_buf))
+        ctx->cb_data(ctx->out_buf, ctx->cb_arg);
+    
+    // was that the last piece of PNG data?
+    if (ctx->png_done && evbuffer_get_length(ctx->out_buf) == 0) {
+        // PNG data done!
+        _render_multi_do_png_done(ctx);
+
+    }
+}
+
+// the PNG rendering completed succesfully
+static void _render_multi_do_png_done (struct render_multi *ctx) {
+    // check that ctx is still valid
+    assert(ctx->alive);
+
+    // mark as not alive
+    ctx->alive = 0;
+
+    // call cb_done
+    ctx->cb_done(ctx->cb_arg);
+
+    // don't free ourself, our user does that (probably did already)
     return;
 }
 
-// the request compelted normally
+// the request completed normally, flush the png data and return
+// _render_multi_png_done takes care of calling cb_done, not us!
 static void _render_multi_do_done (struct render_multi *ctx) {
+    assert(ctx->alive && ctx->png_info);
+
     int i;
-
+    
     // check that all the remote_renders are indeed complete
     for (i = 0; i < ctx->remote_render_count; i++) {
-        assert(ctx->remote_renders[i].remote_render == NULL);
+        assert(ctx->remote_renders[i].render_remote == NULL);
         assert(ctx->remote_renders[i].col == 0);
     }
-
-    // finish the png_info
+    
+    // we need to make sure this render_png_done call only happens once (render_multi_do_fail, state bugs)
     if (render_png_done(ctx->png_info)) {
-        // don't free it twice, though...
         ctx->png_info = NULL;
 
         ERROR("render_png_done");
     }
-
+    
     ctx->png_info = NULL;
 
-    // check that both callbacks are still valid
-    assert(ctx->cb_fail && ctx->cb_done);
+    // mark the png as done now
+    ctx->png_done = 1;
 
-    // call cb_done and then invalidate it
-    ctx->cb_done(ctx->cb_arg);
-    ctx->cb_fail = NULL;
+    // if that all the data handled now, we're done
+    _render_multi_do_png_data(ctx);
 
-    // free ourself
-    _render_multi_do_free(ctx);
-    
-    // ok
+    // don't free ourself, our user does that (probably already did, via render_png_done)
     return;
 
 error:    
-    /* O_o */
+    /* render_png_done failed, probably because we didn't have enough data */
     _render_multi_do_fail(ctx, FAIL_PARTIAL);
 }
 
@@ -170,11 +225,17 @@
 static void _render_multi_do_fail (struct render_multi *ctx, int flags) {
     int i;
 
+    // check that ctx is still valid
+    assert(ctx->alive || flags & FAIL_PARTIAL);
+
+    // mark as not alive
+    ctx->alive = 0;
+
     // cancel any in-progress remote renders
     for (i = 0; i < ctx->remote_render_count; i++)
-        if (ctx->remote_renders[i].remote_render) {
-            render_remote_cancel(ctx->remote_renders[i].remote_render);
-            ctx->remote_renders[i].remote_render = NULL;
+        if (ctx->remote_renders[i].render_remote) {
+            render_remote_cancel(ctx->remote_renders[i].render_remote);
+            ctx->remote_renders[i].render_remote = NULL;
         }
     
     if (!(flags & FAIL_PARTIAL) || ctx->png_info) {
@@ -193,8 +254,7 @@
 
     ctx->cb_fail = NULL;
 
-    // free ourselves
-    _render_multi_do_free(ctx);
+    // don't free ourself, our user does that
 }
 
 /*
@@ -204,60 +264,86 @@
  */
 static void _render_multi_sent (void *arg) {
     struct render_multi_sub *ctx = arg;
-
+    
+    // mark these as sent
     ctx->render_sent = 1;
+    ctx->self->renders_sent++;
 
     // have all render_sub_ctxs been sent?
-    int i;
-    for (i = 0; i < ctx->self->remote_render_count; i++) {
-        if (!ctx->self->remote_renders[i].render_sent)
-            break;
-    }
-    
-    // did we loop through all of them?
-    if (i == ctx->self->remote_render_count) {
+    if (ctx->self->renders_sent == ctx->self->remote_render_count) {
         // tell our user
         _render_multi_do_sent(ctx->self);
     }
 }
 
 /*
- * We have received data from a single render node.
- *
- * Move the data into the row buffer if there's space in it, otherwise
- * leave it inside the buffer.
- *
- * If this fills up our slice, check and see if the entire row is now full. If
- * it is, pass the row to render_png_row, clear out the col values, and call
- * render_remote_shake for each remote render
+ * One render node failed, abort the whole thing
  */
-static void _render_multi_data (struct evbuffer *buf, void *arg) {
+static void _render_multi_fail (void *arg) {
     struct render_multi_sub *ctx = arg;
 
-    assert(ctx->col <= ctx->slice_width);
-
-    // are we being called from _render_multi_done?
-    if (ctx->render_done && ctx->in_buf == NULL) {
-        // move all the data into our own buffer
-        if (!(ctx->in_buf = evbuffer_new()))
-            ERROR("evbuffer_new");
+    // free this ctx's remote render
+    render_remote_free(ctx->render_remote);
+    ctx->render_remote = NULL;
+    
+    // cancel the rest + cb_fail
+    _render_multi_do_fail(ctx->self, 0);
+}
 
-        if (evbuffer_add_buffer(ctx->in_buf, buf))
-            ERROR("evbuffer_add_buffer");
-        
-        // don't do anything else
+/*
+ * Got new data for some remote render
+ */
+static void _render_multi_data_raw (int fd, short event, void *arg) {
+    struct render_multi_sub *ctx = arg;
+    int ret;
+
+    assert(ctx->col <= ctx->slice_width);   // check it isn't out of range
+
+    // if our slice is full, we don't want to receive any more data
+    if (ctx->col == ctx->slice_width)
         return;
-    }
 
-    // is our slice full?
-    if (ctx->col == ctx->slice_width) {
-        // don't care for new data
+    // read new data into our slice
+    ret = read(fd, 
+        ctx->self->rowbuf + ctx->row_offset + ctx->col, // our fixed offset + partial row offset
+        ctx->slice_width - ctx->col                     // how many bytes left in the window
+    );
+    
+    // errors/EOF?
+    if (ret == -1) {
+        if (errno == EAGAIN) {
+            // false alarm
+            goto reschedule;
+
+        } else
+            ERROR("read");
+
+    } else if (ret == 0) {
+        // mark it as done
+        ctx->render_done = 1;
+        ctx->self->renders_done++;
+       
+        // this ctx's remote render is done
+        render_remote_done(ctx->render_remote);
+        ctx->render_remote = NULL;
+
+        // is the data incomplete?
+        if (!(ctx->col == ctx->slice_width || ctx->col == 0))
+            ERROR("incomplete data for slice %d: %zu/%zu bytes", ctx->index, ctx->col, ctx->slice_width);
+
+        // are all of them done?
+        if (ctx->self->renders_done == ctx->self->remote_render_count) {
+            // finish it off
+            _render_multi_do_done(ctx->self);
+
+        } // else, just wait for the rest to complete
+        
+        // do *NOT* reschedule ourself, ctx->render_remote is invalid anyways (as is ctx!)
         return;
     }
     
-    // read new data into our slice
-//    printf("rowbuf + %4d : %d");
-    ctx->col += evbuffer_remove(buf, ctx->self->rowbuf + ctx->row_offset + ctx->col, ctx->slice_width - ctx->col);
+    // ok, we received some data normally
+    ctx->col += ret;
 
     // is our slice full now?
     if (ctx->col == ctx->slice_width) {
@@ -268,33 +354,25 @@
                 break;
         }
         
-        // are they all full?
         if (i == ctx->self->remote_render_count) {
             // pass the data to render_png, this results in calls to _render_multi_png_data
             if (render_png_row(ctx->self->png_info, ctx->self->rowbuf))
                 ERROR("render_png_row");
 
-            // clear the col values
+            // clear the col values and reschedule the reads in case they were paused
             for (i = 0; i < ctx->self->remote_render_count; i++) {
                 ctx->self->remote_renders[i].col = 0;
-            }
-            
-            // shake the buffers
-            // XXX: this will result in recursion
-            for (i = 0; i < ctx->self->remote_render_count; i++) {
-                if (ctx->self->remote_renders[i].remote_render) {
-                    render_remote_flush(ctx->self->remote_renders[i].remote_render);
-                } else {
-                    // already disconnected, use in_buf instead
-                    assert(ctx->self->remote_renders[i].in_buf);
-
-                    // call it directly...
-                    _render_multi_data(ctx->self->remote_renders[i].in_buf, &ctx->self->remote_renders[i]);
-                }
+                render_remote_reschedule(ctx->self->remote_renders[i].render_remote);
             }
         }
     }
 
+    // ok, reschedule ourselves
+
+reschedule:
+    // reschedule a new call once we get more data
+    render_remote_reschedule(ctx->render_remote);
+
     return;
 
 error:
@@ -309,13 +387,12 @@
 static int _render_multi_png_data (const unsigned char *data, size_t length, void *arg) {
     struct render_multi *ctx = arg;
 
-    // XXX: avoid these many data copies?
+    // XXX: need a better user-API to avoid these data copies
     if (evbuffer_add(ctx->out_buf, data, length))
         ERROR("evbuffer_add");
     
-    // only call the data cb if we've already called cb_sent
-    if (ctx->have_sent)
-        ctx->cb_data(ctx->out_buf, ctx->cb_arg);
+    // handle cb_data/cb_done
+    _render_multi_do_png_data(ctx);
     
     // ok
     return 0;
@@ -325,59 +402,6 @@
     return -1;
 }
 
-/*
- * The render node has rendered everything that it needs to render. Our slice
- * of the last row should be full (or empty) now. If all the remote_renders are
- * done, we can call render_png_done and then cb_done.
- */
-static void _render_multi_done (void *arg) {
-    struct render_multi_sub *ctx = arg;
- 
-    // mark it as done
-    ctx->render_done = 1;
-
-    // shake out the rest of the data as needed, as render_multi won't keep it anymore
-    render_remote_flush(ctx->remote_render);
-   
-    // invalidate this ctx's remote render
-    ctx->remote_render = NULL;
-
-    // is the data incomplete?
-    if (!(ctx->col == ctx->slice_width || ctx->col == 0))
-        ERROR("incomplete data for slice %d: %zu/%zu bytes", ctx->index, ctx->col, ctx->slice_width);
-
-    // are all of them done?
-    int i;
-    for (i = 0; i < ctx->self->remote_render_count; i++) {
-        if (!ctx->self->remote_renders[i].render_done)
-            break;
-    }
-    
-    // are they all done?
-    if (i == ctx->self->remote_render_count) {
-        // finish it off
-        _render_multi_do_done(ctx->self);
-    }
-
-    // ok, wait for the rest to complete
-    return;
-    
-error:
-    _render_multi_do_fail(ctx->self, 0);
-}
-
-/*
- * One render node failed, abort the whole thing
- */
-static void _render_multi_fail (void *arg) {
-    struct render_multi_sub *ctx = arg;
-
-    // invalidate this ctx's remote render
-    ctx->remote_render = NULL;
-
-    _render_multi_do_fail(ctx->self, 0);
-}
-
 #define ROUND_DOWN(dividend, divisor) ((dividend) / (divisor))
 #define ROUND_UP(dividend, divisor) (((dividend) / (divisor)) + ((dividend) % (divisor)))
 
@@ -461,12 +485,15 @@
     
     // the two render_remote calls
     if (
-            !(ctx->remote_renders[0].remote_render = render_remote(&r_left, node_left, 
-                &_render_multi_sent, &_render_multi_data, &_render_multi_done, &_render_multi_fail, &ctx->remote_renders[0]))
-         || !(ctx->remote_renders[1].remote_render = render_remote(&r_right, node_right,
-                &_render_multi_sent, &_render_multi_data, &_render_multi_done, &_render_multi_fail, &ctx->remote_renders[1]))
+            !(ctx->remote_renders[0].render_remote = render_remote_rawio(&r_left, node_left, 
+                &_render_multi_sent, &_render_multi_fail, &_render_multi_data_raw, &ctx->remote_renders[0]))
+         || !(ctx->remote_renders[1].render_remote = render_remote_rawio(&r_right, node_right,
+                &_render_multi_sent, &_render_multi_fail, &_render_multi_data_raw, &ctx->remote_renders[1]))
     )
         ERROR("render_remote");
+
+    // we are now alive
+    ctx->alive = 1;
     
     // I guess that's a succesfull start now
     return ctx;
@@ -477,17 +504,21 @@
     return NULL;
 }
 
+
+void render_multi_set_recv (struct render_multi *ctx, size_t recv_threshold, size_t unread_buffer) {
+
+}
+
+int render_multi_flush (struct render_multi *ctx) {
+    _render_multi_do_png_data(ctx);
+
+    return 0;
+}
+
 void render_multi_cancel (struct render_multi *ctx) {
     _render_multi_do_fail(ctx, FAIL_SILENT);
 }
 
-int render_multi_set_recv (struct render_multi *ctx, size_t recv_threshold, size_t unread_buffer) {
-    return 0;
+void render_multi_free (struct render_multi *ctx) {
+    _render_multi_do_free(ctx);
 }
-
-int render_multi_shake (struct render_multi *ctx) {
-    // call the data cb
-    ctx->cb_data(ctx->out_buf, ctx->cb_arg);
-
-    return 0;
-}
--- a/render_multi.h	Sun Jun 08 21:03:23 2008 +0300
+++ b/render_multi.h	Sun Jun 08 23:10:36 2008 +0300
@@ -31,24 +31,12 @@
         void *cb_arg
 );
 
-/*
- * Cancel the given request. No more callbacks will be called, buffered data is
- * discarded and the remote render processes will cancel ASAP.
- */
 void render_multi_cancel (struct render_multi *ctx);
 
-/*
- * Doesn't actually do anything yet
- */
-int render_multi_set_recv (struct render_multi *ctx, size_t recv_threshold, size_t unread_buffer);
+void render_multi_set_recv (struct render_multi *ctx, size_t recv_threshold, size_t unread_buffer);
 
-/*
- * Call cb_data with the current set of buffered input data immediately,
- * regardless of whether or not the buffer contains any data, or any new
- * data has been received.
- *
- * Only call this after cb_sent and before cb_done/cb_fail.
- */
-int render_multi_shake (struct render_multi *ctx);
+int render_multi_flush (struct render_multi *ctx);
+
+void render_multi_free (struct render_multi *ctx);
 
 #endif /* RENDER_MULTI_H */
--- a/render_remote.c	Sun Jun 08 21:03:23 2008 +0300
+++ b/render_remote.c	Sun Jun 08 23:10:36 2008 +0300
@@ -62,6 +62,8 @@
 static void _render_remote_do_fail (struct render_remote *ctx);
 
 static void _render_remote_free (struct render_remote *ctx) {
+    assert(ctx && !ctx->alive);
+
     // free the bev_data
     if (ctx->bev_data)
         bufferevent_free(ctx->bev_data);
@@ -251,7 +253,10 @@
     ctx->cb_sent = cb_sent;
     ctx->cb_fail = cb_fail;
     ctx->cb_arg = cb_arg;
-    
+ 
+    // set up the custom EV_READ callback
+    event_set(&ctx->ev_data, ctx->sock, EV_READ, cb_io_data, cb_arg);
+   
     // set up the write bufferevent
     if ((ctx->bev_data = bufferevent_new(ctx->sock, NULL, &_remote_write, &_remote_error, ctx)) == NULL)
         ERROR("bufferevent_new");
@@ -260,9 +265,6 @@
     if (bufferevent_enable(ctx->bev_data, EV_WRITE))
         ERROR("bufferevent_enable");
 
-    // set up the custom EV_READ callback
-    event_set(&ctx->ev_data, ctx->sock, EV_READ, cb_io_data, cb_arg);
-
     // we are now alive
     ctx->alive = 1;
 
@@ -349,11 +351,23 @@
     // we must be alive for this..
     assert(ctx->alive);
 
+    ctx->alive = 0;
+
+    _render_remote_free(ctx);
+}
+
+void render_remote_done (struct render_remote *ctx) {
+    // we must be alive and non-buffered for this..
+    assert(ctx->alive && event_initialized(&ctx->ev_data));
+
+    ctx->alive = 0;
+
     _render_remote_free(ctx);
 }
 
 void render_remote_free (struct render_remote *ctx) {
-    // XXX: add some sanity checks
+    // must be dead already
+    assert(!ctx->alive);
     
     _render_remote_free(ctx);
 }
--- a/render_remote.h	Sun Jun 08 21:03:23 2008 +0300
+++ b/render_remote.h	Sun Jun 08 23:10:36 2008 +0300
@@ -127,6 +127,12 @@
 void render_remote_cancel (struct render_remote *ctx);
 
 /*
+ * Mark a non-buffered custom-I/O request as succesfully completed. This will
+ * also free the given ctx.
+ */
+void render_remote_done (struct render_remote *ctx);
+
+/*
  * Free the render_remote ctx and its assoicated resources. Only valid after a
  * cb_done/cb_fail, otherwise use render_remote_cancel.
  */
--- a/web_main.c	Sun Jun 08 21:03:23 2008 +0300
+++ b/web_main.c	Sun Jun 08 23:10:36 2008 +0300
@@ -44,7 +44,7 @@
 
     int headers_sent;
     
-    struct render_remote *render_info;
+    struct render_multi *render_info;
 
     size_t bytes_sent;
 
@@ -56,7 +56,7 @@
 
 static void _render_cleanup (struct render_request *ctx) {
     if (ctx->render_info)
-        render_remote_free(ctx->render_info);
+        render_multi_free(ctx->render_info);
 
     free(ctx);
 }
@@ -81,7 +81,7 @@
 
     size_t buf_size = EVBUFFER_LENGTH(buf);
 
-    assert(buf_size > 0);   // shouldn't happen anymore with the new render_remote
+    assert(buf_size > 0);   // shouldn't happen anymore with the new render_multi
     
     // check if we are paused
     if (ctx->paused) {
@@ -137,7 +137,7 @@
     printf("render [%p]: lost http connection\n", ctx);
 
     // cancel
-    render_remote_cancel(ctx->render_info);
+    render_multi_cancel(ctx->render_info);
     ctx->render_info = NULL;
 
     _render_cleanup(ctx);
@@ -152,7 +152,7 @@
     ctx->paused = 0;
 
     // any data waiting in the buffer?
-    render_remote_flush(ctx->render_info);
+    render_multi_flush(ctx->render_info);
 }
 
 static void _http_render_execute (struct evhttp_request *request, u_int32_t img_w, u_int32_t img_h) {
@@ -178,14 +178,8 @@
     if (render_region_full(&render))
         ERROR("render_region_full");
 
-    // pick a render_node
-    struct remote_node *node_info;
-
-    if ((node_info = remote_pool_get(&remote_pool)) == NULL)
-        ERROR("remote_pool_get");
-
     // initiate the remote render operation
-    if ((ctx->render_info = render_remote(&render, node_info,
+    if ((ctx->render_info = render_multi(&render, &remote_pool,
         &_render_sent,
         &_render_data,
         &_render_done,
@@ -195,7 +189,7 @@
         ERROR("render_multi");
     
     // set chunk size
-    render_remote_set_recv(ctx->render_info, MIN_CHUNK_SIZE, OVERFLOW_BUFFER);
+    render_multi_set_recv(ctx->render_info, MIN_CHUNK_SIZE, OVERFLOW_BUFFER);
 
     // set close cb
     evhttp_set_reply_abortcb(request, &_render_http_lost, ctx);