[Spice-devel] [PATCH spice-gtk 14/15] Remove some large stack allocations

[Daniel P. Berrange]

On Tue, Mar 13, 2012 at 02:56:07PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 03/13/2012 02:40 PM, Daniel P. Berrange wrote:
> >From: "Daniel P. Berrange"<berrange at redhat.com>
> >
> >A few functions have very large arrays declared on the stack.
> >Replace these with heap allocations, to reduce risk of stack
> >overflows in deep callpaths
> >---
> >  gtk/channel-playback.c |    6 ++++--
> >  gtk/spice-channel.c    |   16 ++++++++++++----
> >  2 files changed, 16 insertions(+), 6 deletions(-)
> >
> >diff --git a/gtk/channel-playback.c b/gtk/channel-playback.c
> >index 32f8b1a..c353cd2 100644
> >--- a/gtk/channel-playback.c
> >+++ b/gtk/channel-playback.c
> >@@ -353,10 +353,12 @@ static void playback_handle_data(SpiceChannel *channel, SpiceMsgIn *in)
> >                            packet->data, packet->data_size);
> >          break;
> >      case SPICE_AUDIO_DATA_MODE_CELT_0_5_1: {
> >-        celt_int16_t pcm[256 * 2];
> >+        celt_int16_t *pcm;
> >+        gsize pcmLen = 256 * 2;
> >
> >          g_return_if_fail(c->celt_decoder != NULL);
> >
> >+        pcm = g_new0(celt_int16_t, pcmLen);
> >          if (celt051_decode(c->celt_decoder, packet->data,
> >                             packet->data_size, pcm) != CELT_OK) {
> >              g_warning("celt_decode() error");
> >@@ -364,7 +366,7 @@ static void playback_handle_data(SpiceChannel *channel, SpiceMsgIn *in)
> >          }
> >
> >          emit_main_context(channel, SPICE_PLAYBACK_DATA,
> >-                          (uint8_t *)pcm, sizeof(pcm));
> >+                          (uint8_t *)pcm, pcmLen);
> >          break;
> >      }
> >      default:
> 
> You seem to be never freeing the data here. Also I disagree with switching
> to malloc / free here in general. This path will get called many times a second,
> malloc / free is not a cheap operation and 1024 bytes is not such a big amount to
> put on the stack.

[snip]

> This one looks fine from the free pov. But again this is a semi hot
> code path, and malloc / free is not a cheap operation, also there are
> no guarantees wrt cache hotness when it comes to the heap, where as the stack
> is likely hot in the cache.

Ok, no worries, I can tweak the limit in m4/spice-compile.warnings.m4 to
allow these larger stack allocations.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|