[Top][All Lists]
[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