[darcs-devel] darcs patch: Change a can't happen error to an imposs... (and 5 more)

Ian Lynagh igloo at earth.li
Tue Apr 12 20:55:24 PDT 2005


On Tue, Apr 12, 2005 at 07:38:17AM -0400, David Roundy wrote:
> On Mon, Apr 11, 2005 at 11:08:54PM -0400, Ian Lynagh wrote:
> > 
> > One problem we have left is that the skipped list is keeping the list
> > around while we work on the selected patches. If in Record.do_record you
> > change skipped to _skipped and pass [] in its place then memory usage is
> > far better (and time improves again too).
> > 
> > Unfortunately I can't see a nice fix right now, so I'm going to bed
> > instead  :-)
> 
> Hmmmm.  Sleep good!
> 
> We only need skipped in order to write pending changes, so we could pass
> (sift_for_pending skipped) to do_record, but I'm not sure that would help.
> I'm not really clear on why skipped is causing other patches to be held in
> memory... you *are* referring to tests where --all is specified?

Yes. I've talked to Simon Marlow about this and it turns out the
optimisation GHC does to cope with these things isn't working in this
case. I think I've convinced him it can be done, but he wants to mull
over the details. The problem is this; consider the following code:

foo (x:xs) = let (ys, zs) = foo xs
             in if p x then (x:ys, zs) else (ys, x:zs)

where we first demand all of ys (and write_patch them), then all of zs
(and write_pending them), at the top level.

We're interested in the case where p x is always True. The let gets
translated to something like

let e = foo xs
    ys = case e of (as, bs) -> as
    zs = case e of (as, bs) -> bs

so although ys gets evaluated and we descend inside it, zs is still
holding on to everything.


Hopefully we'll soon have a proper fix for this.
One of the patches I've made does a hideous hack with IORefs to get
around it. It's safe with the record code, but I haven't checked it's
safe everywhere else it ends up getting used...
(in fact, the add test is now reported as "dubious" (what a delightful
word to use!), so if I haven't broken anything accidentally then this
isn't safe).


I've also made the application of the patch when recording read the
patch as a String so we get laziness. I'm not sure what effect this has
on runtime.


The timestamp synching seems to be using a silly amount of memory. I
wonder if it's accidentally reading all the files or something.


Oh, and compiling with profiling seems to halve memory usage during at
least the patch writing phase (at least if you avoid -auto-all in
SelectChanges and Record). Which is odd.


> I guess maybe it has to do with how in with_any_selected_changes we handle
> the --all case.  Perhaps we need to special-case the situation where 
> 
> All `elem` opts && null fs
> 
> since in this case other_ps is guaranteed to be [], and shouldn't "hold on"
> to any of the changes.  Also, this is quite a common "special" case, when
> you record --all without specifying a patch name.
> 
> But perhaps there is a non-special-casing way we could rewrite the
> (ps_to_consider, other_ps) bit to make it better-behaved...

I'd definitely like to avoid special casing wherever possible.

> Actually, my timing test just finished running, and my "kernel add
> everything" test now runs in about 435m max RAM, and takes another 20% less
> wallclock time.  This is about as good as the improved "get" performance,
> so it looks like you've already gotten "record" down to the point where
> it's no longer a memory bottleneck.

What exactly are you "get"ting here? A kernel repo with a single initial
patch?


Thanks
Ian





More information about the darcs-devel mailing list