On 12/30/11 13:19, Shawn Walker wrote:
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?
I didn't. Are you thinking of installing them into an empty image or
some other situation? If you describe the experiment you'd like to see,
I'm happy to run it.
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.
Well, all I can say is that all the tests pass with this change. If
there's other reasons not to have facets there, I'm not aware of the
history. I can revert that change, but it means making the checks in the
manifest code more complex (to allow more restrictive excludes to be
provided but not less restrictive ones).
At the very least, the all_variants variable is now misnamed and
needs to be changed. Perhaps 'use_excludes' instead?
Sure, I can change this.
lines 3383-3390: I think this docstring may need an update because
of the changes made here.
You're right, I'll update it.
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.
That's a good idea, I'll do that.
line 3554: I'm concerned that this change may significantly increase
memory usage for long-lived clients such as the packagemanager and
updatemanager.
Can you describe what experiment you'd like me to perform here? On my
x86 machine, this change increases memory usage by 6M, or about 1.2%
compared to the gate.
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.
Ok. I simply kept the code as it was but I can make that change.
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.
I'll try it out and see.
line 1670: can __do_checks be renamed to something more descriptive?
Perhaps __do_check_conflict_existing, __do_check_installed_conflicts
or the like?
Sure.
line 1782: nitty, but you name fmri_dict with an underscore and
offsetdict without...
I'll add an underscore.
line 2072: nit, seems like the retrieval of the new manifest could
be shared between the two cases
I think the code's easier to understand this way.
src/modules/facet.py:
=====================
lines 23-24: copyright needs update
Ok.
lines 111-113, 131-133: nothing needed
here?
131-133 yep, I missed that. 111-113 goes through the __setitem__ code,
so nothing's needed there.
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.
I like it this way because it's safer to me. Right now, in theory the
list could contain any number of elements and we have no way of
identifying what those elements are. That means that we can't do any
safety checking wrt manifests which have excluded their content. That'd
mean I'd need to rollback basically all the changes to the manifest code
which I don't see as a positive. Personally, I vastly prefer this
approach which (in my view) provides a bit of additionaly safety and
performance in exchange for forcing the caller to be clear about what
they want to exclude. The caller can always pass an empty list if they
don't want to exclude variants or facets.
I'm happy to update the docstrings.
-Shawn
Thanks for taking a look.
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
