[Spice-devel] [spice-server PATCH] RFC: stream-device: do not reallocate dev->msg for each frame

[Uri Lublin]

On 07/24/2018 01:25 PM, Frediano Ziglio wrote:
>>
>> In the past dev->msg was only allocated for cursor images, and
>> after use dev->msg size was reduced.
>>
>> Now, since commit dcc3f995d9f55, dev->msg is used for every frame.
>> There is little point of calling reallocate with a large dev->msg size
>> and then reallocate with a small size, for every frame.
>>
>> This patch keeps only the "enlarge buffer when needed" part.
>>
>> The drawback of this approach is that if a big buffer was used
>> it is kept big (at least, in the near future), even if
>> current frames are smaller.
>>
>> Signed-off-by: Uri Lublin <uril at redhat.com>
> 
> Why RFC?

So people can comment :)

I think the benefit (not re-allocating for each frame) is more valuable
than the drawback (keeping a large buffer).

> 
> Looking at current code looks like for similar usage we use a static
> "small" (some KB) buffer as most messages are small and do dynamic
> alloc/free for every other message.

The static buffer is only used upon an error.
All messages are read into dev->msg->buf.

Also once streaming starts there are a lot of consecutive
"DATA" messages (frames).

Thanks,
     Uri.



> 
>> ---
>>   server/red-stream-device.c | 14 --------------
>>   1 file changed, 14 deletions(-)
>>
>> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
>> index d293dc1cc..eda6e6690 100644
>> --- a/server/red-stream-device.c
>> +++ b/server/red-stream-device.c
>> @@ -175,14 +175,6 @@ stream_device_partial_read(StreamDevice *dev,
>> SpiceCharDeviceInstance *sin)
>>        * the next message */
>>       if (handled) {
>>           dev->hdr_pos = 0;
>> -
>> -        // Reallocate message buffer to the minimum.
>> -        // Currently the only message that requires resizing is the cursor
>> shape,
>> -        // which is not expected to be sent so often.
>> -        if (dev->msg_len > sizeof(*dev->msg)) {
>> -            dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
>> -            dev->msg_len = sizeof(*dev->msg);
>> -        }
>>       }
>>   
>>       if (handled || dev->has_error) {
>> @@ -314,12 +306,6 @@ handle_msg_data(StreamDevice *dev,
>> SpiceCharDeviceInstance *sin)
>>       }
>>   
>>       /* make sure we have a large enough buffer for the whole frame */
>> -    /* ---
>> -     * TODO: Now that we copy partial data into the buffer, for each frame
>> -     * the buffer is allocated and freed (look for g_realloc in
>> -     * stream_device_partial_read).
>> -     * Probably better to just keep the larger buffer.
>> -     */
>>       if (dev->msg_pos == 0) {
>>           dev->frame_mmtime = reds_get_mm_time();
>>           if (dev->msg_len < dev->hdr.size) {
> 
> Frediano
>