[Spice-devel] [RFCv5 00/47] server: multiple client support

[Yaniv Kaul]

On 05/22/2011 08:38 PM, Yonit Halperin wrote:
> Hi,
>
> Though implementing this feature using multiple instances of the core data structures, one instance per client, is straightforward
> and comprises a clean solution, I think that we should implement a more parsimonious solution with respect to memory and cpu, so we can support a large number of clients.
> We can keep only one instance of the surfaces, trees, GLZ dictionary, etc., and for each client we will hold a pipe.
> In addition, we should avoid re-encoding the same bitmap for each client it is being sent to, and instead keep the encoded result and send it instantly.
> This solution raises other issues that need handling, e.g.:
> * When decoding a GLZ image, the dictionary at the client side must be the same as the one that was at the server side at the moment of encoding that image. However, the stream of images that is sent to each client is not necessarily identical (due to "covering" of rendering commands that haven't been sent yet), and thus, the dictionary in the server might not be consistent with the ones in the clients. One possible solution is to enforce identical dictionary in all the clients: (1) if an image was previously removed from a client sending queue, we will not use GLZ when sending it to another client (2) if an image was previously sent to a client using GLZ, we will enforce sending it to all the other clients.
> * the pixmap hash at the server side is not an heavy memory consumer, but if we decide to keep only on copy for it, we need to keep it consistent with all the clients, similarly to the GLZ dictionary.
> * For supporting jpeg compresison in WAN, we will need to store surface_client_lossy_regions for each client.
>
> Cheers,
> Yonit.
>
> p.s.
> this feature made me think that separating our channels' logic (mainly the red_worker), from the sending queue(s), to different threads, may enhance Spice's performance, even for one client. Why shouldn't we try to send data in the pipe to the client while we are processing commands pulled from the driver?

It's a good idea, but that's only because we send every update from the 
server to the client. Had we done some queuing and update aggregation, 
we would have had less to send, in larger update windows (but possibly 
still in a single packet, for example). However, separating the work to 
two components could help exactly here - I assume there will be some 
queue between them (say, a producer-consumer pattern) - and the consumer 
can intelligently delay and/or batch updates.
(and if we ever want to multi-thread the producer, I guess that's 
essential as well).

Y.

> ----- Original Message -----
> From: "Alon Levy"<alevy at redhat.com>
> To: spice-devel at freedesktop.org
> Sent: Sunday, May 8, 2011 3:10:56 PM
> Subject: [Spice-devel] [RFCv5 00/47] server: multiple client support
>
> changes from previous (v4) after Marc-Andre's review:
>   Surfaces struct renamed to RedRender
>   patch count reduction.
>   reduction of argument count (makes search replace patches more readable too)
>   some other cleanups. I'm sure I missed a few.
>   to turn on multiple client support you need to set SPICE_DEBUG_ALLOW_MC=1
>   (or any value).
>   tested the handle_dev patch, it seems rendering is not correct with this
>   patch series, see comment on last patch.
>
> Still a lot of todos.
>
> Tree: git://anongit.freedesktop.org/spice/spice server_multi_client.v5
>
> Alon Levy (47):
>    server/main_channel.c: set NET_TEST_STAGE_INVALID=0 explicitly (TODO
>      - the rest)
>    server/red_channel: renames to use _proc postfix consistently
>    server/red_worker: drop red_pipe_add_tail, use
>      red_channel_pipe_add_tail
>    server/red_channel (all): introduce RedChannelClient
>    server/red_worker: introduce {display,cursor}_is_connected
>    server: move pipe from RedChannel to RedChannelClient
>    server/main_channel: use MainChannel in sig
>    server/red_channel: workaround for fast client disconnect bug (TODO -
>      real fix)
>    server/red_client: clear pipe on disconnect
>    server/red_worker: release PIPE_ITEM_TYPE_VERB in display channel
>    server/main_channel+reds: make main_channel_init return
>      MainChannelClient
>    server: Add RedClient
>    server/reds: add concept of secondary channels
>    server/main_channel: move latency and bitrate to channel client
>    server/main_channel: move ping here from reds.
>    server/main_channel: move connection_id from reds
>    server/red_channel: ignore error if already shutdown
>    server/red_channel: introduce pipes functions
>    server/main_channel: support multiple clients
>    server/inputs_channel: support multiple clients
>    server/red_tunnel_worker: trivial multi client support
>    server/smartcard: support multiple clients
>    server/red_worker: split RedRender from RedWorker
>    server/red_worker: cleanup
>    server/red_worker: split display and cursor channels
>    server/red_worker: remove more direct access to RedChannelClient.rcc
>    server/red_channel: add pipe_size helpers
>    server/red_channel: introduce client ring in RedChannel
>    server/reds: add RedsState.allow_multiple_clients
>    server/red_worker: introduce RedSurfaceReleaseInfo
>    server/red_worker: add ref counting to RedDrawable
>    server/red_worker: start using RENDER_FOREACH
>    server/red_worker: whitespace fixes
>    server/red_worker: copy and free new surfaces after first client
>    server/red_worker: get_streams_timeout: go over all clients
>    server/red_worker: move free from display_channel_send_item to
>      _release_item
>    server/red_worker: split cursor pipe item from cursor item
>    server/red_worker: remove forced disconnect on connect
>    server/red_worker: add cursor_channel_client_disconnect
>    server/red_worker: red_current_add_equal: add dcc null checks
>    server/red_worker: on_new_display_channel_client: push ack, cleanup
>    server/red_worker: give each client a different id for caches
>    server/red_worker: DEBUG_CURSORS
>    (temp) server/reds: use ALLOW_NO_SSL define externally to trigger
>      debug only DEBUG_GENERATE_NULL_KEY
>    (temp) server/reds: DEBUG: add a define for working with no SSL (good
>      for valgrind)
>    (temp) client/red_channel: DEBUG: allow no SSL usage (useful for
>      valgrind)
>    server/red_worker: handle_dev_update: for all clients (checkme)
>
>   client/red_channel.cpp           |   40 +-
>   server/inputs_channel.c          |  151 +-
>   server/main_channel.c            |  626 ++++---
>   server/main_channel.h            |   47 +-
>   server/red_channel.c             | 1027 ++++++++---
>   server/red_channel.h             |  195 ++-
>   server/red_client_cache.h        |   58 +-
>   server/red_client_shared_cache.h |   48 +-
>   server/red_dispatcher.c          |   39 +-
>   server/red_dispatcher.h          |    7 +-
>   server/red_parse_qxl.h           |    1 +
>   server/red_tunnel_worker.c       |  403 +++--
>   server/red_worker.c              | 3877 +++++++++++++++++++++++---------------
>   server/red_worker.h              |    2 +
>   server/reds.c                    |  198 ++-
>   server/reds.h                    |   16 +-
>   server/smartcard.c               |  196 +-
>   server/snd_worker.c              |   24 +-
>   18 files changed, 4365 insertions(+), 2590 deletions(-)
>