monotone-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: xdelta speedups (was Re: [Monotone-devel] updates to net.venge.monot


From: Eric Anderson
Subject: Re: xdelta speedups (was Re: [Monotone-devel] updates to net.venge.monotone.experiment.performance)
Date: Thu, 10 Aug 2006 01:06:03 -0700

Nathaniel Smith writes:
 > [ many cosmetic fixups ]

Repaired in 40accd16c45a4195e207b91c1d9dec6d6a8170cb; I think I got
all of them.  Is there a CodingStyle file somewhere that explains all
the rules that should be applied so I can just verify them when I
split out to a patch against mainline?

On Wed, Aug 09, 2006 at 10:42:37PM -0700, Eric Anderson wrote:
 > I am unsure about this, simply because I don't know what widen<> is
 > for in the first place.  Graydon, can you shed any light?

My best guess was that widen was there to make sure that when widening
a signed to an unsigned the right thing happened, but I wasn't sure it
seemed unnecessary for an unsigned to unsigned widen.

 > +  // selfcheck by making short-distance test true || badvance <= blocksz
 > +  //              u32 save_sum = rolling.sum();
 > +  rolling.replace_with(reinterpret_cast<u8 const *>(b.data() + new_lo), 
 > new_hi-new_lo);
 > +  //              I(rolling.sum() == save_sum);
 > 
 > Comments here make me go "huh?".

I expanded the comment a lot, it's just a way to verify that the
skip-forward checksum matches the rolling checksum.  It passed on a
complete pull of the monotone database, but I thought others might
want to verify.

 > +  static bool widen_checked = false;
 > +  if (!widen_checked) {
 > +    for(u32 i = 0; i < 256; ++i) {
 > +      u8 v = (u8)(i & 0xFF);
 > +      u32 v_w = widen<u32,u8>(v);
 > +      I(v_w == i);
 > +      I(((u32)(v)) == i);
 > +    }
 > +    widen_checked = true;
 > +  }
 > 
 > Non-house style again, plus some comments about what's going on here
 > would be good... also I still don't know what widen<> is for, so I
 > still can't comment on whether this code is useful or sane :-).

This code is just there to verify that widen is unnecessary, I put in
a comment about that into the checkin.

 > Other than these concerns, seems fine.  Once we get a resolution to
 > the widen<> thing, and these minor bits are fixed, should be fine to
 > land on mainline.

I'll move it to a separate branch as a single patch once the widen<>
question is resolved.
        -Eric





reply via email to

[Prev in Thread] Current Thread [Next in Thread]