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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-04-30 at 13:23 +0100, Daniel Kiper wrote:
> On Mon, Apr 29, 2013 at 12:16:43PM +0100, Ian Campbell wrote:
> > 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.
> 
> OK.
> 
> > 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.
> 
> Here are four things:
>   - reduction of number of variables which you found,
>   - removal of unneeded intialization; probably it was
>     needed some time ago but now it is not,
>   - removal of memorykb = new_target_memkb assignment
>     with relevant changes in other places,
>   - merge of video memory size calculation which
>     is currently in two places.
> 
> I do not insist on first thing but I think that
> other optimizations are worth to do. If you wish
> I could do all of them in separate patches.

IMHO we should be optimising this code for readability, not for number
of variables or even performance (this is hardly a hot path).

With that in mind, #1 -- no, #2 ok, #3 might be ok if it improves
readability (but hard to tell with it mixed in with the other fixes), #4
is probably ok on the basis of not repeating the logic.

So separate patches are probably the way to go.


Ian.


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