[darcs-users] darcs patch: add exception to haskell_policy.sh for D... (and 61 more)

[David Roundy]

On Thu, Oct 30, 2008 at 01:09:36PM -0700, Jason Dagit wrote:
> On Thu, Oct 30, 2008 at 1:00 PM, David Roundy <droundy at darcs.net> wrote:
> > On Wed, Oct 29, 2008 at 07:50:32PM +0000, Eric Kow wrote:
> >> On Wed, Oct 29, 2008 at 10:30:12 -0700, Jason Dagit wrote:
> >> > I want to see benchmarks too, but I thought I would justify why we
> >> > expect this to be no slower than the previous code...Everything below
> >> > is stuff that we discussed during the Sprint.
> >>
> >> Well, attached is a second set of comparative timing tests, sorry, only
> >> run once and with no nice output yet.  Hopefully you can use a graphical
> >> diff tool to do side by side comparison.
> >>
> >> A nice little summariser script, maybe using the Haskell tabular
> >> library might be handy
> >
> > I've run my own set of timings, which give considerably more dramatic
> > differences than yours show, perhaps because I ran my tests on large
> > repositories?
> >
> > I'll summarize my results up here, but you can look below for a more
> > verbose summary.  The new code is almost always faster by my reckoning, and
> > often *much* faster (as much as a factor of three!).
> 
> Very nice.  Just curious, how many trial runs did you do?

Three runs with each configuration.  The variation in almost every case was
much less than the difference between the three configurations.

> > I do, however, observe a performance regression on darcs annotate Setup.hs
> > (run in the darcs repository).  It's a small regression (smaller than
> > Eric's tests show), but reproducible.  And it's all the more striking given
> > the dramatic improvement the new code shows in all the other tests, which
> > suggest there may be a single function that has a large performance
> > regression (since it seems likely that parts of the annotate command have
> > been sped up in the new version).  I don't have the time or inclination to
> > track down this issue, but I do hope Don is interested enough to look into
> > it!
> 
> If we're going to accept this, it may be better to look at the
> annotate issue after Ganesh's patches go in.  Otherwise there is
> potential to waste effort optimizing something that is going to be
> removed or rewritten, right?

No, the point is that there's a bytestring primitive that is significantly
slower than it ought to be, and we ought to take this chance to figure out
what that primitive is.  The smart approach would have been to *first*
optimize the bytestring implementation to be as fast as the
OldFastPackedString implementation and *then* remove the old
implementation.  It's quite possible that putting the OldFastPackedString
code back in will give us yet another speedup, because most of the
optimizations Don did were independent of the switch to bytestring
(mostly just fixing stupid things that I wrote).

As it is, the optimizations to the darcs code are mixed up with
pessimizations in the bytestring implementation, and it's close to
impossible to find those pessimizations.  True, they're not all that huge
(maybe no worse than a factor of two or three?), but it's still be nice to
avoid slowing the code down at all, particularly when there's little to no
benefit.  I'm pretty much convinced (having looked at many of the changes
Don made) that the speedup isn't due to the OldFastPackedString ->
bytestring change, but rather to simple optimizations that would affect
either library.
-- 
David Roundy
http://www.darcs.net