On Thu 10 Apr 2008 at 10:54AM, Danek Duvall wrote:
> On Thu, Apr 10, 2008 at 01:25:47AM -0700, Dan Price wrote:
>
> > http://cr.opensolaris.org/~dp/ips-perf
> generic.py
>
> - line 177: is there anything about orderdict that's instance-specific?
> Couldn't we save a teeny bit of time and space by making it specific
> either to the Action class or even the pkg.actions module?
I thought that it was essentially a static element of the class as
declared?
> - line 233: inconsistent use of single-quotes vs backslash-escaping.
> Would it be any slower to use %-expandos -- it'd likely be easier to
> read.
When I was bit twiddling, I found % expandos to be enough slower than string
concatenation to make a difference in hot paths like this one, although my
testing is admittedly limited.
Example:
timeit.Timer('str=" " + k + "=" + v', 'k, v = "foo", "bar"').timeit()
0.587
timeit.Timer('str=" %s=%s" % (k, v)', 'k, v = "foo", "bar"').timeit()
0.949
While in general we should use %, in some hot paths I think
concatenation is the way to go.
> filelist.py
>
> - line 205: to stderr? Though this low-level code probably shouldn't be
> printing at all ...
Ok. I hit this when we found the duplicate license actions, and this
was originally just debug code. I'll remove for now.
> image.py
>
> - line 896, 902: I'd probably rename gen_installed_pkgs() to
> installed_pkgs(), and gen_installed_pkgs_forreal() to
> gen_installed_pkgs(). That means renaming all the clients, but I think
> it's a better naming scheme. We can postpone that if you like.
Yes, let's please defer that. I thought the convention was that
things which can be used as generators had gen_ in them?
> manifest.py
>
> - line 257: I guess I was thinking that the actions should be agnostic in
> regards to the information they carried, while the manifest was more
> part of the system, where the idea that paths shouldn't start with the
> root filesystem was more relevant, and that the "path" attribute was
> universal across actions that represented filesystem objects.
Ok-- so, should I push this out into the File, Dir, etc. actions? Since
path is commonly specified, this isn't a performance concern, more just
a question I had when I found this code. I'll remove the comment.
> - line 267: won't this have problems if self.size isn't set first?
I may be confused-- but self.size is set to 0 in the constructor.
> - line 315: what if this fails?
Well, we'll throw. Pretty sure this is the same as the previous
behavior from before.
I accepted and fixed the rest of your comments-- thanks much.
-dp
--
Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
