[darcs-users] darcs patch: Fix a memory leak in Darcs.Repository.Repair.
ganesh at earth.li
Tue Dec 23 00:59:15 UTC 2008
On Tue, 23 Dec 2008, Petr Rockai wrote:
> As it were, it built a FL of patches, possibly replacing some with a
> repaired version. However, since it also applied those patches, their
> content was evaluated and never freed (since the list was needed at the
> end of repair). What I have done now is, that we only remember the
> patches that were actually changed, and then re-create the list of all
> patches, and only then, I zip in those changed patches.
> Since the new, re-created list of patches stays unevaluated (well, the contents
> part of it, anyway), we avoid the massive memory use of the original code.
> The obvious problem with my current patch is the way replaceInFL works,
> relying on PatchInfo equality on the repaired patches. I unfortunately
> haven't had time to verify that this is always true, or whether we
> really want to enforce this as a policy. Consider this patch a DRAFT
> until we decide upon this issue. Also, Jason pointed out safety issues
> in replaceInFL and proposed use of IsEq. I currently don't have time to
> grok IsEq (I need to sleep!), but Ganesh might shed some light into that
> issue and with his help, I might be able to add some safety to that
Just to give a (very brief!) overview of the point of IsEq: the type
witnesses stuff is designed so that the type system generally knows when
two patch contexts are the same, because they are represented by the same
type witness. Sometimes, this information (namely the equality of two
contexts) has to be propagated around the darcs code to help the type
system out, and the way we do this is with IsEq, which amounts to evidence
that two type witnesses with different names are actually the same. The
point about it is the declaration:
data EqCheck a b where
IsEq :: EqCheck a a
NotEq :: EqCheck a b
so if we have some value of type EqCheck a b in our hands, and we inspect
it and it turns out that it is 'IsEq', then we have a proof that a = b.
There are cases where the type system just can't keep track of
equality no matter how much we help it, and I think this is one of them.
The issue here is that the type witnesses won't match up between the list
re-read in from the repo and the list of changed patches, for two reasons.
Firstly, when we put the changed patches in a list, we lose track of the
type witnesses they originally had. Secondly, re-reading from the repo
will generate a fresh set of witnesses in any case. It's only our
knowledge that reading the same repo twice generates the same sequence of
contexts that tells us that the code is ok.
I guess the proposed patch just won't work with type witnesses as it
stands, and will need whatever an appropriate "unsafe" operation to make
it compile. I think lispy was suggesting on #darcs that you manufacture an
IsEq with unsafeCoerce (i.e. just 'unsafeCoerce IsEq') in a utility
function that compares the pinfos, but I might have misunderstood. I can
try and help with the details if you can catch me on IRC.
More information about the darcs-users