On 12/30/11 14:05, Brock Pytlik wrote:
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.
Yes, the empty image case. Imagine an AI install being done, and try to
simulate that.
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).
Not asking for that, just racking my brain trying to figure out why I'm
uneasy about this. As I said, my feeling of unease may no longer apply.
But I know for certain I considered the same change at an earlier
point and after talking with Bart or Danek (don't remember which) came
to the conclusion that it was intentional and shouldn't be changed.
But if the test suite passes (I was already assuming it did), then I
can't give any specific objection.
So many parts of the system should have had more comments :-(
...
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.
I was thinking what would happen to the packagemanager if a user ran a
check for update all, but didn't complete the operation and then
continued to do other things. But I suppose since the process peak
memory usage will never be reduced once planning is performed, I
shouldn't expect much of a difference.
You've addressed the primary concern then -- the stat() of the offsets
file to ensure the cache is correct.
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.
Thanks for that.
...
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.
I don't have a better alternative, so updating the docstrings seems like
the right change at a minimum for now. It does mean that callers of the
class will have to pass an empty set of excludes for facets if they
don't want to apply any, but that doesn't seem too terrible.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
