Re: [Xen-devel] [PATCH v4 3/4] libxl: Some optimizations in libxl_set_memory_target()

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-04-29 at 12:09 +0100, Daniel Kiper wrote:
> Some optimizations in libxl_set_memory_target().

Please describe them in the commit message.

At first glance it just looks like you are renaming some variables from
"memmax"/"target"/"uuid" to "s", why? Just to reduce the number of local
variables?

They seem more meaningful and easier to read with their current names
and if you are "optimising" the use of local variable then that seems
like a false optimisation to me -- any modern compiler can surely work
out that the lifetimes of these various things do not overlap and do the
sensible thing WRT register allocation.

At the very least this renaming is hiding what you are actually doing.

> 
> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> ---
>  tools/libxl/libxl.c |   48 ++++++++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 26 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 3427c13..683b700 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3640,21 +3640,18 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t 
> domid,
>  {
>      GC_INIT(ctx);
>      int rc = 1, abort_transaction = 0;
> -    uint32_t guestkb, memorykb = 0, videoram = 0;
> -    uint32_t current_target_memkb = 0, new_target_memkb = 0;
> -    char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
> -    char *dompath = libxl__xs_get_dompath(gc, domid);
> +    uint32_t current_target_memkb, guestkb, memorykb = 0, new_target_memkb;
> +    char *dompath = libxl__xs_get_dompath(gc, domid), *endptr, *s;
>      xc_domaininfo_t info;
>      libxl_dominfo ptr;
> -    char *uuid;
>      xs_transaction_t t;
>  
>  retry_transaction:
>      t = xs_transaction_start(ctx->xsh);
>  
> -    target = libxl__xs_read(gc, t, libxl__sprintf(gc,
> +    s = libxl__xs_read(gc, t, libxl__sprintf(gc,
>                  "%s/memory/target", dompath));
> -    if (!target && !domid) {
> +    if (!s && !domid) {
>          xs_transaction_end(ctx->xsh, t, 1);
>          rc = libxl__fill_dom0_memory_info(gc, &current_target_memkb);
>          if (rc < 0) {
> @@ -3662,18 +3659,18 @@ retry_transaction:
>              goto out;
>          }
>          goto retry_transaction;
> -    } else if (!target) {
> +    } else if (!s) {
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
>                  "cannot get target memory info from %s/memory/target\n",
>                  dompath);
>          abort_transaction = 1;
>          goto out;
>      } else {
> -        current_target_memkb = strtoul(target, &endptr, 10);
> +        current_target_memkb = strtoul(s, &endptr, 10);
>          if (*endptr != '\0') {
>              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
>                      "invalid memory target %s from %s/memory/target\n",
> -                    target, dompath);
> +                    s, dompath);
>              abort_transaction = 1;
>              goto out;
>          }
> @@ -3688,21 +3685,21 @@ retry_transaction:
>  
>      xcinfo2xlinfo(&info, &ptr);
>  
> -    memmax = libxl__xs_read(gc, t, libxl__sprintf(gc,
> +    s = libxl__xs_read(gc, t, libxl__sprintf(gc,
>                  "%s/memory/guest-max", dompath));
>  
> -    if (memmax) {
> -        guestkb = strtoul(memmax, &endptr, 10);
> +    if (s) {
> +        guestkb = strtoul(s, &endptr, 10);
>          if (*endptr != '\0')
>              guestkb = 0;
>      } else
>          guestkb = 0;
>  
>      if (!guestkb) {
> -        memmax = libxl__xs_read(gc, t, libxl__sprintf(gc,
> +        s = libxl__xs_read(gc, t, libxl__sprintf(gc,
>                      "%s/memory/static-max", dompath));
>  
> -        if (!memmax) {
> +        if (!s) {
>              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
>                      "cannot get memory info from %s/memory/static-max\n",
>                      dompath);
> @@ -3710,12 +3707,12 @@ retry_transaction:
>              goto out;
>          }
>  
> -        memorykb = strtoul(memmax, &endptr, 10);
> +        memorykb = strtoul(s, &endptr, 10);
>  
>          if (*endptr != '\0') {
>              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
>                      "invalid max memory %s from %s/memory/static-max\n",
> -                    memmax, dompath);
> +                    s, dompath);
>              abort_transaction = 1;
>              goto out;
>          }
> @@ -3752,18 +3749,14 @@ retry_transaction:
>          abort_transaction = 1;
>          goto out;
>      }
> -    videoram_s = libxl__xs_read(gc, t, libxl__sprintf(gc,
> -                "%s/memory/videoram", dompath));
> -    videoram = videoram_s ? atoi(videoram_s) : 0;
>  
>      if (enforce) {
> -        memorykb = new_target_memkb;
> -        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
> +        rc = xc_domain_setmaxmem(ctx->xch, domid, new_target_memkb +
>                  LIBXL_MAXMEM_CONSTANT);
>          if (rc != 0) {
>              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
>                      "xc_domain_setmaxmem domid=%d memkb=%d failed "
> -                    "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, rc);
> +                    "rc=%d\n", domid, new_target_memkb + 
> LIBXL_MAXMEM_CONSTANT, rc);
>              abort_transaction = 1;
>              goto out;
>          }
> @@ -3775,7 +3768,10 @@ retry_transaction:
>          goto out;
>      }
>  
> -    new_target_memkb -= videoram;
> +    s = libxl__xs_read(gc, t, libxl__sprintf(gc,
> +                "%s/memory/videoram", dompath));
> +    new_target_memkb -= s ? atoi(s) : 0;
> +
>      rc = xc_domain_set_pod_target(ctx->xch, domid,
>              new_target_memkb / 4, NULL, NULL, NULL);
>      if (rc != 0) {
> @@ -3790,8 +3786,8 @@ retry_transaction:
>      libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target",
>                  dompath), "%"PRIu32, new_target_memkb);
>  
> -    uuid = libxl__uuid2string(gc, ptr.uuid);
> -    libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
> +    s = libxl__uuid2string(gc, ptr.uuid);
> +    libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", s),
>              "%"PRIu32, new_target_memkb / 1024);
>  
>  out:



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel