[Spice-devel] [PATCH spice-gtk v2 1/2] Change the setting of the images cache size and the glz window size

Tue Jan 24 00:01:21 PST 2012

Hi,

On 01/23/2012 12:36 PM, Hans de Goede wrote:
> Hi,
>
> I've various remarks, see my comments inline.
>
> On 01/22/2012 09:12 AM, Yonit Halperin wrote:
>> Set the default sizes to be the same as in the old linux spice client.
>> cache_size=20M pixels (instead of 32M), window_size=8M pixels for a 64MB
>> dev ram (instead of 16M pixels).
>> ---
>> gtk/channel-display-priv.h | 2 -
>> gtk/channel-display.c | 22 +++++++---
>> gtk/channel-main.c | 1 +
>> gtk/spice-session-priv.h | 12 ++++++
>> gtk/spice-session.c | 90 +++++++++++++++++++++++++++++++++++++++++++-
>> 5 files changed, 117 insertions(+), 10 deletions(-)
>>
>> diff --git a/gtk/channel-display-priv.h b/gtk/channel-display-priv.h
>> index 332d271..eca1787 100644
>> --- a/gtk/channel-display-priv.h
>> +++ b/gtk/channel-display-priv.h
>> @@ -36,8 +36,6 @@
>>
>> G_BEGIN_DECLS
>>
>> -#define DISPLAY_PIXMAP_CACHE (1024 * 1024 * 32)
>> -#define GLZ_WINDOW_SIZE (1024 * 1024 * 16)
>>
>> typedef struct display_surface {
>> RingItem link;
>> diff --git a/gtk/channel-display.c b/gtk/channel-display.c
>> index ec42829..2473ac6 100644
>> --- a/gtk/channel-display.c
>> +++ b/gtk/channel-display.c
>> @@ -766,13 +766,21 @@ static void emit_invalidate(SpiceChannel
>> *channel, SpiceRect *bbox)
>> static void spice_display_channel_up(SpiceChannel *channel)
>> {
>> SpiceMsgOut *out;
>> - SpiceMsgcDisplayInit init = {
>> - .pixmap_cache_id = 1,
>> - .pixmap_cache_size = DISPLAY_PIXMAP_CACHE,
>> - .glz_dictionary_id = 1,
>> - .glz_dictionary_window_size = GLZ_WINDOW_SIZE,
>> - };
>> -
>> + SpiceSession *s = spice_channel_get_session(channel);
>> + SpiceMsgcDisplayInit init;
>> + int cache_size;
>> + int glz_window_size;
>> +
>> + g_object_get(s,
>> + "cache-size",&cache_size,
>> + "glz-window-size",&glz_window_size,
>> + NULL);
>> + SPICE_DEBUG("%s: cache_size %d, glz_window_size %d (bytes)",
>> __FUNCTION__,
>> + cache_size, glz_window_size);
>> + init.pixmap_cache_id = 1;
>> + init.glz_dictionary_id = 1;
>> + init.pixmap_cache_size = cache_size / 4; /* pixels */
>> + init.glz_dictionary_window_size = glz_window_size / 4; /* pixels */
>> out = spice_msg_out_new(channel, SPICE_MSGC_DISPLAY_INIT);
>> out->marshallers->msgc_display_init(out->marshaller,&init);
>> spice_msg_out_send_internal(out);
>> diff --git a/gtk/channel-main.c b/gtk/channel-main.c
>> index ebf660f..b2df547 100644
>> --- a/gtk/channel-main.c
>> +++ b/gtk/channel-main.c
>> @@ -1128,6 +1128,7 @@ static void main_handle_init(SpiceChannel
>> *channel, SpiceMsgIn *in)
>> init->current_mouse_mode);
>>
>> spice_session_set_mm_time(session, init->multi_media_time);
>> + spice_session_set_caches_hints(session, init->ram_hint,
>> init->display_channels_hint);
>>
>> c->agent_tokens = init->agent_tokens;
>> if (init->agent_connected)
>> diff --git a/gtk/spice-session-priv.h b/gtk/spice-session-priv.h
>> index 5df1182..a814cfe 100644
>> --- a/gtk/spice-session-priv.h
>> +++ b/gtk/spice-session-priv.h
>> @@ -27,6 +27,10 @@
>>
>> G_BEGIN_DECLS
>>
>> +#define IMAGES_CACHE_SIZE_DEFAULT (1024 * 1024 * 80)
>> +#define MIN_GLZ_WINDOW_SIZE_DEFAULT (1024 * 1024 * 12)
>> +#define MAX_GLZ_WINDOW_SIZE_DEFAULT MIN((LZ_MAX_WINDOW_SIZE * 4),
>> 1024 * 1024 * 64)
>> +
>
> Are these glz window size min and max values only for the default
> calculation, or
> should the same min / max also be applied in
> spice_session_set_property() ? If they
> also should be applied in spice_session_set_property, I suggest dropping
> the
> _DEFAULT, and adding checks for them there. If they should not be
> applied in
> spice_session_set_property(), I would like to see some additional
> MIN/MAX defines
> which are used to clamp the input value in spice_session_set_property(),
> likewise I
> would like to see some min / max defines for the image cache size and the
> PROP_CACHE_SIZE input value clamped to these in
> spice_session_set_property().
>
The min/max default values are used only for the default calculation.
In g_object_class_install_property I set the min/max value for both the 
cache and dictionary. Their minimal size is 0. The maximal size for the 
cache is G_MAXINT and for the dictionary it is the maximal value that 
can be processed by the lz code:  LZ_MAX_WINDOW_SIZE * 4 ( 
LZ_MAX_WINDOW_SIZE is in pixels). These limits will allow us to test a 
variety of values. Once we have conclusion for the optimal sizes 
(depending on different conditions), we can set stricter limits.
Do you want me to add explicit defines for these limits?

>> struct _SpiceSessionPrivate {
>> char *host;
>> char *port;
>> @@ -84,6 +88,11 @@ struct _SpiceSessionPrivate {
>> display_cache images;
>> display_cache palettes;
>> SpiceGlzDecoderWindow *glz_window;
>> + int images_cache_size;
>> + int glz_window_size;
>> + uint32_t pci_ram_size;
>> + uint32_t display_channels_count;
>> +
>>
>> /* associated objects */
>> SpiceAudio *audio_manager;
>> @@ -120,6 +129,9 @@ const gchar*
>> spice_session_get_cert_subject(SpiceSession *session);
>> const gchar* spice_session_get_ciphers(SpiceSession *session);
>> const gchar* spice_session_get_ca_file(SpiceSession *session);
>>
>> +void spice_session_set_caches_hints(SpiceSession *session,
>> + uint32_t pci_ram_size,
>> + uint32_t display_channels_count);
>> void spice_session_get_caches(SpiceSession *session,
>> display_cache **images,
>> display_cache **palettes,
>> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
>> index 79ba1b6..d36709e 100644
>> --- a/gtk/spice-session.c
>> +++ b/gtk/spice-session.c
>> @@ -101,6 +101,8 @@ enum {
>> PROP_DISABLE_EFFECTS,
>> PROP_COLOR_DEPTH,
>> PROP_READ_ONLY,
>> + PROP_CACHE_SIZE,
>> + PROP_GLZ_WINDOW_SIZE,
>> };
>>
>> /* signals */
>> @@ -407,7 +409,13 @@ static void spice_session_get_property(GObject
>> *gobject,
>> break;
>> case PROP_READ_ONLY:
>> g_value_set_boolean(value, s->read_only);
>> - break;
>> + break;
>> + case PROP_CACHE_SIZE:
>> + g_value_set_int(value, s->images_cache_size);
>> + break;
>> + case PROP_GLZ_WINDOW_SIZE:
>> + g_value_set_int(value, s->glz_window_size);
>> + break;
>> default:
>> G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
>> break;
>> @@ -508,6 +516,12 @@ static void spice_session_set_property(GObject
>> *gobject,
>> s->read_only = g_value_get_boolean(value);
>> g_object_notify(gobject, "read-only");
>> break;
>> + case PROP_CACHE_SIZE:
>> + s->images_cache_size = g_value_get_int(value);
>> + break;
>> + case PROP_GLZ_WINDOW_SIZE:
>> + s->glz_window_size = g_value_get_int(value);
>> + break;
>> default:
>> G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
>> break;
>> @@ -913,6 +927,36 @@ static void
>> spice_session_class_init(SpiceSessionClass *klass)
>> G_PARAM_CONSTRUCT |
>> G_PARAM_STATIC_STRINGS));
>>
>> + /**
>> + * SpiceSession:cache-size:
>> + *
>> + * Images cache size. If 0, don't set.
>> + *
>> + **/
>> + g_object_class_install_property
>> + (gobject_class, PROP_CACHE_SIZE,
>> + g_param_spec_int("cache-size",
>> + "Cache size",
>> + "Images cache size (bytes)",
>> + 0, G_MAXINT, 0,
>> + G_PARAM_READWRITE |
>> + G_PARAM_STATIC_STRINGS));
>> +
>> + /**
>> + * SpiceSession:glz-window-size:
>> + *
>> + * Glz window size. If 0, don't set.
>> + *
>> + **/
>> + g_object_class_install_property
>> + (gobject_class, PROP_GLZ_WINDOW_SIZE,
>> + g_param_spec_int("glz-window-size",
>> + "Glz window size",
>> + "Glz window size (bytes)",
>> + 0, LZ_MAX_WINDOW_SIZE * 4, 0,
>> + G_PARAM_READWRITE |
>> + G_PARAM_STATIC_STRINGS));
>> +
>> g_type_class_add_private(klass, sizeof(SpiceSessionPrivate));
>> }
>>
>> @@ -1664,3 +1708,47 @@ void spice_session_get_caches(SpiceSession
>> *session,
>> if (glz_window)
>> *glz_window = s->glz_window;
>> }
>> +
>> +G_GNUC_INTERNAL
>> +void spice_session_set_caches_hints(SpiceSession *session,
>> + uint32_t pci_ram_size,
>> + uint32_t display_channels_count)
>> +{
>> + SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
>> +
>> + g_return_if_fail(s != NULL);
>> +
>> + s->pci_ram_size = pci_ram_size;
>> + s->display_channels_count = display_channels_count;
>> +
>> + /* TODO: when setting cache and window size, we should consider the
>> client's
>> + * available memory and the number of display channels */
>> + if (s->images_cache_size == 0) {
>> + s->images_cache_size = IMAGES_CACHE_SIZE_DEFAULT;
>> + }
>> +
>> + if (s->glz_window_size == 0) {
>> + s->glz_window_size = MIN(MAX_GLZ_WINDOW_SIZE_DEFAULT, pci_ram_size /
>> 2);
>> + s->glz_window_size = MAX(MIN_GLZ_WINDOW_SIZE_DEFAULT,
>> s->glz_window_size);
>> + }
>> +}
>> +
>
> You're not using the 2 functions below, nor are you adding them to any
> header file, instead you're using g_object_get above, which I believe is
> the better way to handle this. Please drop these 2 functions.
>
Yeah, forgot to remove them when I removed them from the header. Thanks.

Cheers,
Yonit.

>> +G_GNUC_INTERNAL
>> +int spice_session_get_images_cache_size(SpiceSession *session)
>> +{
>> + SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
>> +
>> + g_return_val_if_fail(s != NULL, 0);
>> + g_return_val_if_fail(s->images_cache_size != -1, 0);
>> + return s->images_cache_size;
>> +}
>> +
>> +G_GNUC_INTERNAL
>> +int spice_session_get_glz_window_size(SpiceSession *session)
>> +{
>> + SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
>> +
>> + g_return_val_if_fail(s != NULL, 0);
>> + g_return_val_if_fail(s->glz_window_size != -1, 0);
>> + return s->glz_window_size;
>> +}
>
> Regards,
>
> Hans