On 12/29/11 19:45, Brock Pytlik wrote:
Webrev:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/performance
Bugs:
19104 CliTestCase.pkgsend needs to display traceback when pkgsend
tracesback
19113 old dictionary in imageplan.__find_all_conflicts should be seeded
using old_excludes
19119 generic.get_varcet_keys shouldn't use startswith
19120 Image shouldn't load the action dictionary twice during an update
19121 if actions.offsets includes number of lines with the same key,
things can be faster
19122 pkgplans should hold onto their manifests a bit longer
19123 maintaining a cache of created fmris while checking for
conflicting actions makes things faster
19124 conflicting actions should use sets of strings instead of PkgFmris
19125 pkg_solver should stop creating the same pfmris over and over
19126 manifests should track what's been excluded from them
19127 compiling re's in facets is faster than using fnmatch
I see that you provided lots of numbers for differences in update, but
did you check install for solaris-small-server and the like?
src/modules/client/image.py:
============================
lines 2454-2455: I'm uncertain about this change though those
concerns may be for reasons that are no longer valid. I would swear
this was discussed before and it was discovered that this intentionally
didn't take facets into account. That may have been before we changed
imageplanning to do conflict checking though and the like.
At the very least, the all_variants variable is now misnamed and
needs to be changed. Perhaps 'use_excludes' instead?
lines 3383-3390: I think this docstring may need an update because of
the changes made here.
line 3513-3514: It seems like this needs a stat check on the offsets
file at a minimum to ensure the data is still valid, so you need to
stash the file timestamp as well. My concern is for long-lived clients
like the packagemanager/updatemanager.
line 3554: I'm concerned that this change may significantly increase
memory usage for long-lived clients such as the packagemanager and
updatemanager.
src/modules/client/imageplan.py:
================================
line 1595: "5.11" shouldn't be hard-coded here; instead, you should
assign build-release outside of the main loop and then re-use inside:
brelease = self.image.attrs["Build-Release"]
Yes, there are still places that hard-code it, but I've been fixing
them as I change related areas. The publication tools are an exception
since they're not tied to an image for now.
lines 1598-1599: I'm not sure how much of a difference it will make
here, but I've discovered in the past that setdefault() can be fairly
slow in a hot path. Instead, you may find this construct faster:
try:
old[key].append((act, pfmri))
except (KeyError, AttributeError):
old[key] = [(act, pfmri)]
It all depends on how many times the entry already exists. On SPARC
especially, this tends to be noticeably faster since we avoid an extra
function call. You use this construct in a few other places as well.
line 1670: can __do_checks be renamed to something more descriptive?
Perhaps __do_check_conflict_existing, __do_check_installed_conflicts
or the like?
line 1782: nitty, but you name fmri_dict with an underscore and
offsetdict without...
line 2072: nit, seems like the retrieval of the new manifest could be
shared between the two cases
src/modules/facet.py:
=====================
lines 23-24: copyright needs update
lines 111-113, 131-133: nothing needed
here?
src/modules/manifest.py:
========================
I'm not thrilled with the idea of forcing both variant and facet
excludes to always be specified. They can be legitimately done
independently. Why the assert; it's not actually a bug to not provide
both. I'm not strictly opposed to it, but at the very least, all the
docstrings need updating where they mention excludes.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
