[PATCH blivet/master 1/3] devicelibs.lvm: refactor _getConfigArgs()/lvm()

Anne Mulhern amulhern at redhat.com
Tue Sep 23 18:49:01 UTC 2014





----- Original Message -----
> From: "Will Woods" <wwoods at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Tuesday, September 23, 2014 12:25:38 PM
> Subject: Re: [PATCH blivet/master 1/3] devicelibs.lvm: refactor	_getConfigArgs()/lvm()
> 
> On Mon, 2014-09-22 at 08:14 -0400, Anne Mulhern wrote:
> > Moving _getConfigArgs() into lvm seems fine with me.
> > 
> > But, better to extend lvm() in a principled manner than to invent new
> > method lvm_output.
> > Seems more consistent w/ idea of moving _getConfigArgs() call into lvm(),
> > anyway.
> > See example in mdraid.py.
> 
> I'd been thinking of them as analogues to subprocess.{call,check_output}
> but yeah, the mdraid() approach makes sense.
> 
> > Fine w/ READONLY_COMMANDS change, generally, but think variable
> > READONLY_COMMANDS should be local to _getConfigArgs(),
> > not file global.
> 
> I... generally define those outside the scope of the function as a
> (possibly trivial) optimization, i.e. initialize the list once rather
> than on every function call? I wonder if the python interpreter
> optimizes that out?
> 

The example below indicates no:

Source:

GLOBAL_CONSTANT = ('zippy', 'zoopy')

def junk():

    CONSTANT = ('zippy', 'zoopy')

    return CONSTANT[1]

def junk1():

    return GLOBAL_CONSTANT[1]

Assembly:

>>> dis(junk)
Disassembly of junk:
  5           0 LOAD_CONST               4 (('zippy', 'zoopy'))
              3 STORE_FAST               0 (CONSTANT)

  7           6 LOAD_FAST                0 (CONSTANT)
              9 LOAD_CONST               3 (1)
             12 BINARY_SUBSCR       
             13 RETURN_VALUE        

Disassembly of junk1:
 11           0 LOAD_GLOBAL              0 (GLOBAL_CONSTANT)
              3 LOAD_CONST               1 (1)
              6 BINARY_SUBSCR       
              7 RETURN_VALUE 


But in the context of every crazy expensive operation that blivet
performs over and over again, trivial -> nugatory. You definitely should not
be making that decision for performance reasons.

Using a tuple, instead of a list, though, turns out to be useful, because the list
must be built item by item, but the tuple seems to come preassembled in the source.

So, defining the tuple globally could actually be a deoptimization,
as loading a global might be more expensive
than messing with a locally defined constant. Who knows, really?

On the doctrine of the least strength and smallest scope that gets what you need done
(to which I subscribe, clearly), READONLY_COMMANDS defined as a tuple in the method wins.

> > Please make sure to verify relevant tests succeed, you need to be root to
> > run the majority.
> 
> Yup - I mentioned that in the initial email:
> > 'make test' shows no new failures after adding this code.
>

Yeah, thanks for doing that.

> -w
> 
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

- mulhern


More information about the anaconda-patches mailing list