[Spice-devel] [PATCH spice-server 3/6] spicevmc: Avoid computing some variable value if not necessary

[Christophe Fergeau]

On Thu, Jan 26, 2017 at 09:37:56AM -0500, Frediano Ziglio wrote:
> > 
> > On Thu, Jan 26, 2017 at 10:56:45AM +0000, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/spicevmc.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > > index a92ec7f..23e60b3 100644
> > > --- a/server/spicevmc.c
> > > +++ b/server/spicevmc.c
> > > @@ -585,12 +585,13 @@ static uint8_t
> > > *spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
> > >                                                         uint32_t size)
> > >  {
> > >      RedVmcChannel *channel;
> > > -    RedClient *client = red_channel_client_get_client(rcc);
> > > -
> > > -    channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > +    RedClient *client;
> > >  
> > >      switch (type) {
> > >      case SPICE_MSGC_SPICEVMC_DATA:
> > > +        client = red_channel_client_get_client(rcc);
> > > +        channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > +
> > >          assert(!channel->recv_from_client_buf);
> > >  
> > >          channel->recv_from_client_buf =
> > >          red_char_device_write_buffer_get(channel->chardev,
> > > @@ -615,10 +616,9 @@ static void
> > > spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> > >  {
> > >      RedVmcChannel *channel;
> > >  
> > > -    channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > -
> > >      switch (type) {
> > >      case SPICE_MSGC_SPICEVMC_DATA:
> > > +        channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > >          /* buffer wasn't pushed to device */
> > >          red_char_device_write_buffer_release(channel->chardev,
> > >          &channel->recv_from_client_buf);
> > >          break;
> > 
> > This is something that I'd just let the compiler handle if needed. Note
> > that this also change some variables from being always initialized to
> > sometimes being initialized, which is in my opinion is not necessarily a
> > good move.
> > 
> > Christophe
> > 
> 
> In this case the compiler cannot do it. How can it know that not calling
> a function does not make any difference?

So red_channel_client_get_channel() should be annotated with
G_GNUC_PURE so that the compiler knows about this and can optimize
everywhere by itself?
Thinking more about this patch, even though I hate the {} you have to
add, I'd be ok with

spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
{
    switch (type) {
    case SPICE_MSGC_SPICEVMC_DATA: {
          RedVmcChannel *channel;

          channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
          /* buffer wasn't pushed to device */
          red_char_device_write_buffer_release(channel->chardev,
          &channel->recv_from_client_buf);
          break;
    }

which improves the reading of the code in my opinion (you know 'channel'
is not going to be used anywhere else).

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170126/e089d058/attachment.sig>