I've noticed that for some repos bleeding-edge darcs is unbearably slow doing 
"darcs pull"

And here is what I've found:

sh-4.1$ darcs show repo
          Type: darcs
        Format: darcs-1.0
          Root: /home/adept/devel/haskell/darcs-get/cabal
      Pristine: PlainPristine "_darcs/pristine"
         Cache: thisrepo:/home/adept/devel/haskell/darcs-get/cabal, 
cache:/home/adept/.darcs/cache
boringfile Pref: .darcs-boring
Default Remote: http://darcs.haskell.org/cabal
   Num Patches: 2498

sh-4.1$ time darcs-2.4.4 pull                                                 
Pulling from "http://darcs.haskell.org/cabal"...               
This is the Cabal HEAD branch. If you're looking for one of the others
like the 1.2 or 1.6 branch, see http://darcs.haskell.org/cabal-branches/
**********************
No remote changes to pull in!                                            
                                                                         
real    0m6.298s
user    0m0.036s
sys     0m0.044s

sh-4.1$ darcs pull --dry-run --debug-verbose
Beginning identifying repository .
Done identifying repository .
Identified darcs-1 repo: /home/adept/devel/haskell/darcs-get/cabal
Would pull from "http://darcs.haskell.org/cabal"...
Beginning identifying repository http://darcs.haskell.org/cabal
[snip]
Done identifying repository http://darcs.haskell.org/cabal
[snip]
This is the Cabal HEAD branch. If you're looking for one of the others
like the 1.2 or 1.6 branch, see http://darcs.haskell.org/cabal-branches/
**********************
Beginning reading inventory of repository /home/adept/devel/haskell/darcs-get/cabal
Looking for inventory for:
[TAG 1.6.0.1
Duncan Coutts <duncan@haskell.org>**20081011182516] 
Looking for inventory for:
[TAG 1.6.0.1
Duncan Coutts <duncan@haskell.org>**20081011182516] 
URL.copyUrlWithPriority 
(http://darcs.haskell.org/cabal/_darcs/patches/20081011182516-adfee-
12d206c73e78275631cdfea68d1bf37ed75f87a7.gz
                      -> ./darcs22920)
URL.waitUrl http://darcs.haskell.org/cabal/_darcs/patches/20081011182516-adfee-
12d206c73e78275631cdfea68d1bf37ed75f87a7.gz
URL.urlThread (http://darcs.haskell.org/cabal/_darcs/patches/20081011182516-adfee-
12d206c73e78275631cdfea68d1bf37ed75f87a7.gz
            -> ./darcs22920)
URL.requestUrl (http://darcs.haskell.org/cabal/_darcs/patches/20081011182516-adfee-
12d206c73e78275631cdfea68d1bf37ed75f87a7.gz
              -> ./darcs22920)
URL.requestUrl succeeded
URL.waitNextUrl start
URL.waitNextUrl succeeded: 
http://darcs.haskell.org/cabal/_darcs/patches/20081011182516-adfee-
12d206c73e78275631cdfea68d1bf37ed75f87a7.gz ./darcs22920
URL.copyUrlWithPriority 
(http://darcs.haskell.org/cabal/_darcs/patches/20100820120031-adfee-
5a9d3d6612bcf5016da9905e5fa545e7fe963547.gz
                      -> ./darcs22920)
URL.waitUrl http://darcs.haskell.org/cabal/_darcs/patches/20100820120031-adfee-
5a9d3d6612bcf5016da9905e5fa545e7fe963547.gz
URL.urlThread (http://darcs.haskell.org/cabal/_darcs/patches/20100820120031-adfee-
5a9d3d6612bcf5016da9905e5fa545e7fe963547.gz
            -> ./darcs22920)
URL.requestUrl (http://darcs.haskell.org/cabal/_darcs/patches/20100820120031-adfee-
5a9d3d6612bcf5016da9905e5fa545e7fe963547.gz
              -> ./darcs22920)
URL.requestUrl succeeded
Ctrl-C

withSignalsHandled: Interrupted!

sh-4.1$ darcs-2.4.4 --version
2.4.4 (release)

sh-4.1$ darcs --version
2.4.98.3 (+ 210 patches)



As you can see, "darcs pull" re-downloads all patches from the remote repository 
even if there is nothing new to pull (and it says so in the end).

I'm thinking about making a shell test case for this, but don't yet know how to 
make sure that remote darcs-1 I'll set up for the test will remain accessible for 
others as well.

I've made a script for "darcs trackdown" that tests for this and left it 
run through the night.

It tracked down all the way to the darcs-2.4.1 (full history of patches 
undone attached). 

I think that the most suspicious patch is "A new implementation of 
PatchSet and its operations". I'm trying to unpull just this patch and 
verify that, but right now I am overwhelmed by compilation errors.

As soon as I'll be able to get a minimum set of patches to unpull, I'll 
let you know.

Attachments

• trackdown-history.gz

Hi Petr, Dmitry seems to suggest there may be some sort of performance 
regression related to NewSet, so I'm adding you to this ticket for 
interest.

Dmitry: could you help us to narrow down the parameters a bit?  In 
particular: could you help us determine if this is specifically related 
to old-fashioned repositories, or if a hashed clone of the repo also 
behaves the same way.

BTS training: if Dmitry were working in a private repository, then this 
bug would be set waiting-for because there is nothing we can do to 
advance the ticket further.  In this case, however, because Dmitry 
provided so much information, and because it's all public, in principle, 
*any* person interested can do the requested action, so we set it to 
need-action.  It's that we request that Dmitry be that person, but only 
as a question of convenience.  [Lately, I've been trying to make my 
thought process explicit on my BTS activity, so that we can either 
refine the process, or at least codify it]

On Thu, Sep 2, 2010 at 12:23 PM, Eric Kow <bugs@darcs.net> wrote:
>
> Eric Kow <kowey@darcs.net> added the comment:
>
> Hi Petr, Dmitry seems to suggest there may be some sort of performance
> regression related to NewSet, so I'm adding you to this ticket for
> interest.
>
> Dmitry: could you help us to narrow down the parameters a bit?  In
> particular: could you help us determine if this is specifically related
> to old-fashioned repositories, or if a hashed clone of the repo also
> behaves the same way.

I've made tests and observed regression on two repositories:

One:
$ darcs show repo
          Type: darcs
        Format: hashed
(this was made via "darcs get" from darcs-1.0 repo)

Other:
$ darcs show repo
          Type: darcs
        Format: darcs-1.0

So I am confident that both types are affected.

> BTS training: if Dmitry were working in a private repository, then this
> bug would be set waiting-for because there is nothing we can do to
> advance the ticket further.  In this case, however, because Dmitry
> provided so much information, and because it's all public, in principle,
> *any* person interested can do the requested action, so we set it to
> need-action.  It's that we request that Dmitry be that person, but only
> as a question of convenience.  [Lately, I've been trying to make my
> thought process explicit on my BTS activity, so that we can either
> refine the process, or at least codify it]

:)

I've also tried made some progress with "darcs trackdown".

First, I made a script that tests for the attempt to re-download the
patches (by grepping for "get patch" in --debug-verbose output) and
did "darcs trackdown" with it. It took several hours and undid 470
patches, which I though might be a bit more than was actually needed
(list of them is already attached to this ticket).

So next I took a fresh copy of bleeding-edge darcs repo and unpulled
the most-suspicious patch ("A new implementation of PatchSet and its
operations") by hand (along with 138 dependent patches). Result was
not immediately compilable, so I did "darcs trackdown" with "cabal
install" as test command in order to find the closest possible
configuration that will compile. This attempt ended up undoing the
same 470 patches in total (counting 138 that I undid by hand).

I admit that I haven't used trackdown in the past, so I was not aware
that patches are selected in a simple and predictable manner that
would ensure that my second attempt would end with exactly the same
result as the first :)

I've tried to pull/unpull patches manually to get a workable
configuration that omits less patches, but failed.

Do you need more info about the state that I reached with trackdown?
How I could provide it? Should I, perhaps, tag it, and "darcs send"
you the tag?


--
Dmitry Astapov

Hi,

Dmitry Astapov <dastapov@gmail.com> writes:
> First, I made a script that tests for the attempt to re-download the
> patches (by grepping for "get patch" in --debug-verbose output) and
> did "darcs trackdown" with it. It took several hours and undid 470
> patches, which I though might be a bit more than was actually needed
> (list of them is already attached to this ticket).
>
> So next I took a fresh copy of bleeding-edge darcs repo and unpulled
> the most-suspicious patch ("A new implementation of PatchSet and its
> operations") by hand (along with 138 dependent patches). Result was
> not immediately compilable, so I did "darcs trackdown" with "cabal
> install" as test command in order to find the closest possible
> configuration that will compile. This attempt ended up undoing the
> same 470 patches in total (counting 138 that I undid by hand).

I believe this all comes down to the way pull + newset currently
works. It will look at PatchInfoAnd p that came from the remote
repository. If both repositories are hashed, this comes for free, since
the hashes from the remote repo will match the ones we have locally and
nothing is downloaded. However, if the remote is not hashed, it can't
figure the patches are the same and downloads. Booh.

I am not sure it's easy to fix, though. It comes down to findUncommon
doing a partition (through findCommonWithThem) both ways. We need to do
that for --skip-conflicts... the original get_common_and_uncommon code
that worked around this no longer exists.

I think that at this stage, the best thing we can do is document this
and recommend that users that need to interact with oldfashioned
repositories a lot do not upgrade. A real fix would be quite invasive
and would delay 2.5 at best another couple weeks and at worst a couple
of months.

Yours,
   Petr.

On Thu, Sep 02, 2010 at 09:22:44PM +0000, Petr Ročkai wrote:
> 
> I think that at this stage, the best thing we can do is document this
> and recommend that users that need to interact with oldfashioned
> repositories a lot do not upgrade. A real fix would be quite invasive
> and would delay 2.5 at best another couple weeks and at worst a couple
> of months.

If the next darcs release will support darcs1 repos, then I think this
regression is more important than the release schedule.

If it won't support darcs1 repos, then I think it would be better to
remove support, and to release it as darcs 3.


Thanks
Ian

I have looked into this in a bit more detail, and the problem is not nearly as 
severe as it seemed initially. Darcs is not actually downloading all patches, it 
will just fetch the tags from the remote repository. I suspect some issue in 
with_partial_intersection, which is however quite opaque and I'd rather not refactor 
it too wildly on the release branch.

Correction: patches beyond last clean tag are downloaded as well, my test 
repositories all happened to have a clean tag on top.

:(

I'd like to second Ian: there are still a lot of darcs-1 repos around 
(including repository for Cabal, for example), and I, for one, would 
have to keep darcs-2.5 and darcs-2.2 (for older repos) and make sure not 
to mix the two.

And that would be a major PITA.

It seems that the following fixes the problem. Patch against adventure, but should be 
straightforward to backport. It needs cleaning up anyway. The original file is 
src/Darcs/Repository/DarcsRepo.hs, function readRepoPrivate. Took a while to figure, 
but at least I understand the code better now. The problem was that looking just at 
the info forced download of the patch.

diff -rN -u -p old-adventure/src/Darcs/Repository/Old.lhs new-
adventure/src/Darcs/Repository/Old.lhs
--- old-adventure/src/Darcs/Repository/Old.lhs  2010-09-03 18:09:46.000000000 +0200
+++ new-adventure/src/Darcs/Repository/Old.lhs  2010-09-03 18:09:48.000000000 +0200
@@ -152,15 +152,15 @@ readOldRepo k (Repo d _ _ _) = do
                  return $ seal $ Tagged tag00 Nothing ps :<: ts
           parse2 :: RepoPatch p => PatchInfo -> FilePath
                                 -> IO (Sealed (PatchInfoAnd p C(x)))
-          parse2 i fn = do ps <- gzFetchFilePS fn Cachable
-                           return $ hopefullyNoParseError i (toPath fn) (readPatch 
ps)
-          hopefullyNoParseError :: PatchInfo -> String
+          parse2 i fn = do ps <- unsafeInterleaveIO $ do putStrLn $ "grabbed patch: 
" ++ show i
+                                                         gzFetchFilePS fn Cachable
+                           putStrLn $ "parse2 patch: " ++ show i
+                           return $ seal $ patchInfoAndPatch i $ 
hopefullyNoParseError (toPath fn) (readPatch ps)
+          {- hopefullyNoParseError :: PatchInfo -> String
                                 -> Maybe (Sealed (Named a1dr C(x)), b)
-                                -> Sealed (PatchInfoAnd a1dr C(x))
-          hopefullyNoParseError i _ (Just (Sealed x, _)) =
-              seal $ patchInfoAndPatch i $ actually x
-          hopefullyNoParseError i s Nothing =
-              seal $ patchInfoAndPatch i $ unavailable $ "Couldn't parse file "++s
+                                -> Sealed (PatchInfoAnd a1dr C(x)) -}
+          hopefullyNoParseError _ (Just (Sealed x, _)) = actually x
+          hopefullyNoParseError s Nothing = unavailable $ "Couldn't parse file "++s
           read_patches :: RepoPatch p =>
                           (FORALL(b) PatchInfo -> IO (Sealed (PatchInfoAnd p C(b))))
                        -> [PatchInfo] -> IO (Sealed (RL (PatchInfoAnd p) C(x)))


Hopefully (pun?) someone can pick up from there.

Yours,
   Petr.

The following patch updated issue issue1942 with status=resolved;resolvedin=2.8.0 HEAD

* Resolve issue1942: Fix an IO interleaving bug in old-fashioned readRepo. 
Ignore-this: d89716791fecb518f530fcf1b1924a9f

I've just pulled this patch, built the fresh "darcs" and, alas, 
regression is still there for me.

I've tried with darcs-1 repo and with hashed replica of darcs-1 repo - 
patches are still being downloaded. I've tried several times to make 
sure that caches are not playing some role here.

Dmitry Astapov <bugs@darcs.net> writes:

> Dmitry Astapov <dastapov@gmail.com> added the comment:
>
> I've just pulled this patch, built the fresh "darcs" and, alas, 
> regression is still there for me.
>
> I've tried with darcs-1 repo and with hashed replica of darcs-1 repo - 
> patches are still being downloaded. I've tried several times to make 
> sure that caches are not playing some role here.

Indeed, because I broke the fix when fixing type signatures (the
unsealing of hopefullyNoParseError through IO pattern match probably
broke laziness again). I'll give it another try, since there doesn't
seem to be much activity around this.

But, tomorrow, since it's way too late and my build of darcs is broken
right now.

Yours,
   Petr.

01:07:40 | morn@twi:~/dev/darcs/adventure -> darcs diff -p issue1942
Wed Sep  8 01:06:56 CEST 2010  Petr Rockai <me@mornfall.net>
  * Resolve issue1942: fix the fix which ended up too strict due to unsealing.
diff -rN -u -p old-adventure/src/Darcs/Repository/Old.lhs new-
adventure/src/Darcs/Repository/Old.lhs
--- old-adventure/src/Darcs/Repository/Old.lhs  2010-09-08 01:07:46.000000000 
+0200
+++ new-adventure/src/Darcs/Repository/Old.lhs  2010-09-08 01:07:46.000000000 
+0200
@@ -97,7 +97,7 @@ import Darcs.Global ( darcsdir )
 import Darcs.Utils ( catchall )
 import Darcs.ProgressPatches ( progressFL )
 import Printer ( text, (<>), Doc, ($$), empty )
-import Darcs.Witnesses.Sealed ( Sealed(Sealed), seal, unseal )
+import Darcs.Witnesses.Sealed ( Sealed(Sealed), seal, unseal, mapSeal )
 import Darcs.Witnesses.Unsafe( unsafeCoercePEnd )
 import Darcs.Repository.InternalTypes( Repository(..) )
 
@@ -153,8 +153,8 @@ readOldRepo k (Repo d _ _ _) = do
           parse2 :: RepoPatch p => PatchInfo -> FilePath
                                 -> IO (Sealed (PatchInfoAnd p C(x)))
           parse2 i fn = do ps <- unsafeInterleaveIO $ gzFetchFilePS fn Cachable
-                           Sealed p <- return $ hopefullyNoParseError (toPath fn) 
(readPatch ps)
-                           return $ seal $ patchInfoAndPatch i p
+                           return $ patchInfoAndPatch i
+                               `mapSeal` hopefullyNoParseError (toPath fn) 
(readPatch ps)
           hopefullyNoParseError :: String -> Maybe (Sealed (Named a1dr C(x)), b)
                                 -> Sealed (Hopefully (Named a1dr) C(x))
           hopefullyNoParseError _ (Just (Sealed x, _)) = seal $ actually x

(Sadly, this no longer cherrypicks to mainline.)

What's next? Should we wait for Dmitry to try out this change?

What's an "adventure"? Is this some other, more bleeding-edge repo of darcs?

Am I right in assuming that this patch could not be applied to development
darcs even manually?

On Sat, Sep 11, 2010 at 3:21 PM, Reinier Lamers <bugs@darcs.net> wrote:

>
> Reinier Lamers <tux_rocker@reinier.de> added the comment:
>
> What's next? Should we wait for Dmitry to try out this change?
>
> ----------
> status: in-progress -> waiting-for
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/issue1942>
> __________________________________
>



-- 
Dmitry Astapov

Attachments

• unnamed

If Petr is not around, I'll manually import his patch in msg12439 to
mainline.  Simpler would be if he submitted it.

Hi Dmitry, the adventure is a body of work that Petr has been starting
to clean up the Darcs library.

See http://lists.osuosl.org/pipermail/darcs-users/2010-August/025098.html

Hi,

I've manually applied that patch to current darcs repo (I am at a loss 
why I haven't done that earlier) and could confirm that it fixes this 
issues for me - for both darcs-1 and hashed-copy-of-darcs-1 repos.

I've recorded and sent a patch against mainline darcs -- see patch387

As noted in a comment about patch387, this last proposed fix breaks
"darcs tag". Petr, do you know why that could be?

Well, as everyone knows I am not exactly interested in oldfashioned and 
I promised myself to not invest more time in it (already did more for 
this bug than I intended). The bug does not affect adventure since it 
doesn't support writing OF so no darcs tag for OF either.

Anyway, to the matter at hand, I have no idea why darcs tag would break 
here. Nevertheless, IO interleaving is of course unsafe and dangerous 
and is done all over darcs, so it's easily conceivable that something 
somewhere went out of the right order and broke. This would happen e.g. 
if you readRepo, then remove something and then expect the readRepo 
result to work (which it won't, due to delayed IO which expects 
something to exist and it's no longer there). It maybe doesn't break 
with hashed since removing things from hashed works differently. I am 
not really sure, anyway.

If we release Darcs 2.5 with a severe performance regression on a 
commonly operation in old-fashioned, it would make users' lives a lot 
less bearable.

Our options are either to

1. Fix the bug
2. Accept the bug, point out the performance regression, perhaps using 
it as a stick to encourage users to upgrade
3. Not release Darcs 2.5

As a community we cannot just say "I'm not interested; you pick up the 
pieces".  

That said, option #2 could actually help us along the way for 
deprecating old-fashioned by Darcs 2.8.

Petr says "kowey: About 1942 -- as I have already hinted, the tag problem 
can be fixed by wibbling strictness in Commands.Tag."

Can I have a wibbler?

The following patch updated issue issue1942 with status=resolved;resolvedin=2.8.0 HEAD

* Resolve issue1942: fix the fix which ended up too strict due to unsealing. 
Ignore-this: 87012509b3c1c0137306b348ae585059
(manually backpopting fix by Petr Rockai to mainline)

It's not yet resolved on the 2.5 branch. See patch387.

The following patch updated issue issue1942 with status=resolved;resolvedin=2.5.0 CURRENT

* Resolve issue1942: Fix an IO interleaving bug in old-fashioned readRepo. 
Ignore-this: d89716791fecb518f530fcf1b1924a9f