Bug#319377: libapt-pkg: patches for speedup
On Thu, Jul 21, 2005 at 08:06:53PM +0300, Sami Liedes wrote:
> Package: apt
> Version: 0.6.38
> Tags: patch
> Severity: wishlist
>
> Hi,
>
> I profiled aptitude a bit and found out that most of its slowness(*)
> is due to apt (which shouldn't be that much of a surprise, aptitude
> being mostly only a frontend for apt). Here's a patch that speeds up
> some things quite a bit, especially with some optimizations to
> aptitude too (which I will submit separately soon).
I've put your patch into arch, on the branch apt--string-copies--0.
The archive is at
http://people.debian.org/~mdz/arch/apt@packages.debian.org/ as always.
The one bit that I'm uncertain about, after a quick read, is the change to
stringcasecmp(). I'm not sure that it's guaranteed that the locale settings
will always be the same in every context. I think it's probably safe for
the existing code, but risks unexpected behavior.
Did you profile any of these changes separately? What were the objective
results from your changes as a whole?
mdz Integral_: I have ~100 unread in that particular folder
Integral_ oh
Integral_ it's bug #319377
Integral_ I skimmed over it and it looks like it's mostly changes to avoid string copies.
mdz Integral_: hmm, I wonder why it is against such an old version
Integral_ I was thinking that it might be simpler to use a string class that handles immutable strings with refcounting -- it would be less efficient but it would also be harder to re-introduce string copies by accident
Integral_ mdz: I think it might be a typo, the Version: header says 0.6.38
mdz Integral_: libstdc++ still doesn't include a useful string class, does it?
Integral_ mdz: well, it provides one that copies like crazy every time you use it
mdz Integral_: the patch mostly looks reasonable; I'm not sure about the stringcasecmp change wrt locale correctness
Integral_ mdz: Yeah, I think tolower() is allowed to change its behavior in other locales.
Integral_ Or toupper() as the case may be
mdz Integral_: it might not be an issue depending on where stringcasecmp is called; I haven't looked
mdz hmm...surely two .push_back()s shouldn't be significantly different, performance-wise, from += (null-terminated)
Integral_ mdz: grep suggests it's used for version matching, parsing the policy file, and comparing package names.
Integral_ mdz: OTOH, it shows up in strutl.h, so if you're really unlucky, people are using it in the wild
Integral_ mdz: yes, some of the idioms there are weird. I'd also think that insert(s.end(), s2.begin(), s2.end()) should be the same as s += s2.
Integral_ mdz: I think he changed += to push_back in some places because he changed a string to a vector
Integral_ which seems pointless to me
Integral_ unless the g++ implementation of std::string is truly atrocious
mdz Integral_: does that patch actually apply for you? it seems corrupt
mdz rediff gets it to apply, but I'm not sure tha tit's correct
mdz that it's
Integral_ mdz: I haven't tried to apply it, actually
Integral_ mdz: it applies everywhere but init.cc for me
mdz Integral_: likewise
mdz rediff | patch works
Integral_ hm
mvo mdz, Integral_: did you measured the perforance difference?
mvo (resulting from the patch)
mdz Integral_: mind if I copy our conversation to the BTS?
mdz mvo: no, I've done nothing but make it apply to get it into arch
Integral_ mdz: go ahead
--
- mdz
Reply to: