On 01/03/12 15:23, Danek Duvall wrote:
Brock Pytlik wrote:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/performance
image.py:
- You haven't changed the stripped file format, have you?
You're right, I haven't. I'll change its format back to V1.
- I'd like to see the key remain the last field in the offset file, for
readability.
Isn't it? I think the offset file is now:
'name offset count key'
(I had to leave the key as the last item because it can contain spaces
which is what's being used as a delimiter.)
imageplan.py:
- gen_only_new_installed_info (and others): docstring needs updating.
Right.
- Did you look at simply rewriting __gen_star_actions_bytype() instead of
creating __gen_star_actions_bytype_from_extant_manifests()? I know
there'll be ripple effects, but I feel like this probably just adds
clutter to an already gigantic class. You could have callers always
pass in the correct excludes parameter (can you write a test case for
19113?).
It wasn't the excludes that had me create
__gen_star_actions_bytype_from_extant_manifests(), it was the extant
manifest part. Actually, what I need to do is make the gen*info methods
protected/private because they depend upon the manifests not being
cleared from the pkgplans.
- line 1575: use enumerate() instead?
I'll try it. I think there was some reason that I didn't use it, but
I'll check it out.
- line 1600: if you moved this test to the top of the loop and changed it
to a ">", then you could invert the test on line 1580, and dedent the
result of that test instead.
That's true, but it'd mean reading one more line from the file each
time. That's probably not a significant overhead (given buffered reads
etc) so I'll try it out and see if it makes any substantial difference.
- line 1757, 1763: since this is a performance wad anyway, why not use
generator expressions here, rather than list comprehensions?
Sure, I'll try that.
- line 1782: I wonder if this weakrefdict should simply be a member of
the PkgFmri class itself, as it seems like it would be useful in more
places than just here. The constructor could then use it directly.
You might have some issues with some places wanting their own copy of
the object, rather than a reference to an existing one, but if we had
both mutable and immutable fmri objects, that might be easier. And it
would save on parsing time regardless.
I did prototype this during development but it wasn't a win. It actually
slowed things down and caused memory bloat. My guess as to the reason is
that in a lot of situations, we aren't constructing the same FMRI over
and over (for example, reading the catalog), so we end up putting a lot
of things in the dictionary which aren't ever used again and we slow
down fmri construction because we have to fail the table lookup before
we start making the new object. I was surprised by this too, enough that
I might try it once more to confirm my results, but the results were
pretty convincing.
pkg_solver.py:
- line 139: why not a weakrefdict here, too?
Ah, good question. I did try the weakref dict but it didn't perform
nearly as well as the normal dictionary (measured by both time and
number of fmri objects constructed). I believe the reason can be seen on
lines 852 and 1193 where all we take from the PkgFmri object is the
name. I think because we don't hold on to a reference of the PkgFmri we
just created, it gets quickly garbage collected, removing it from the
weakref dictionary before we get to reuse it much. That's probably worth
a comment in the code now that I think about it.
Danek
Thanks for taking a look,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
