----- Original Message -----
From: "Will Woods" <wwoods(a)redhat.com>
To: "anaconda patch review" <anaconda-patches(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern