Sorry for bundling the refactoring with the fix but Darcs.Repository was a
monster. Separating out the concurrency related stuff was a prerequisite for
me to get an idea what's going on.

2 patches for repository http://darcs.net/screened:

patch 109a5667e7cf1f4db879a2aa4076237229f90b09
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Sun Jun 21 23:49:33 CEST 2015
  * refactored Darcs.Repository
  
  All the code implementing cloneRepository and createRepository moved to a
  separate module Darcs.Repository.Clone and only re-exported by
  Darcs.Repository. Only a few small functions remain in Darcs.Repository,
  which was certainly originally intended to be for re-exporting only.
  
  Furthermore, moved the fetching and unpacking code used by cloneRepository
  and createRepository to separate module Darcs.Repository.Packs. This module
  now contains all the tricky concurrency code that I suspect being reponsible
  for causing issue2400.
  
  This patch makes no functional changes. The only changes beside the above
  two large refactorings are
  - culling (and some sorting) of the imports
  - inlining of copyRepoAndGoToChosenVersion into cloneRepository
  - consistent use of the </> operator from System.FilePath
  - combine fetching and unpacking of the packs in a function

patch 466ac99075e1bdcd19cba29dc7c669bae1c15fd1
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Sun Jun 21 23:54:09 CEST 2015
  * resolve issue2400: use async package to keep track of unpack threads
  
  The main difference is that we now cancel all threads when the job is done.
  The previous implementation left one of the threads running and I suspect
  (but haven't strictly verified) that this caused the error message. There is
  rather strong evidence though: turning on debug messages makes the problem
  disappear, as did turning off the concurrency (by commenting out the
  forkIO), both of which suggests a race condition. Then there is the fact
  that the clone actually succeeded despite the error message. Last not least,
  with this patch in effect I can no longer reproduce the problem.

Attachments

• patch-preview.txt
• refactored-darcs_repository.dpatch
• unnamed

I have screened the refactoring patch since it a few other small
refactorings and cleanups depend on it and I don't want to send the same
patch in two different bundles.

I am still unsure what the intention of the original code was w.r.t. the
starting of two parallel threads. I am certain I did not misrepresent
what the code actually did do, namely starting two threads that race for
completion, abandoning (now: canceling) the non-yet-complete one. But is
this really what should happen? One thread fetches all the patches the
function gets as a parameter. The other thread fetches the patches
listed in "meta-filelist-inventories". Are the passed patches garuanteed
to be a subset of what's listed in meta-filelist-inventories? (This is
absolutely not obvious and a small comment explaing the author's
intention when writing this rather tricky code would have made things so
much easier to understand!) Another question is why the thread that
fetches the passed patches is started only when the meta-filelist has
been written and not earlier. I suspect this was an artefact of the old
implementation and could be changed now.

Anyway I am screening this now. At worst the new code is wrong as the
old one. In that case it should be easier to fix because the intention
is clear now and the code more robust.

I will continue to investigate this.

Note to self: find out where else in Darcs forkIO is used and
investigate whether using the async abstraction makes sense there, too.

One patch following up and improving on the one one that claims to have
resolved issue2400. The other patch is a dependency (see patch1380). I
think it makes a lot of sense to apply them all together anyway, also for
2.10.1, so no need to rebase patch1380, which is why I have obsoleted the
latter.

2 patches for repository http://darcs.net/screened:

patch 90657a5991d2c040255c9a31c02e50122df630f5
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Thu Jun 25 00:15:07 CEST 2015
  * resolve issue2459: fall back to writing the file if createLink fails

patch 23bad6777ea545cbf8f5d2856ab3a50d190b0f8d
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Sun Jun 28 20:18:58 CEST 2015
  * remove race from D.R.Packs, further simplify the code
  
  It turned out cancelling the other thread makes the error only less
  probable, I could still reproduce it. The reason is that the race is
  semantically incorrect: The extra patches to fetch are not a subset of the
  ones we get from the pack, since they may contain patches that were added
  after creation of the pack.
  
  In order to further simplify the code and remove possible race conditions,
  the meta-filelist-xxx files are not written to disk. Instead we directly
  evaluate their content and pass it to fetchFilesUsingCache.

Attachments

• patch-preview.txt
• resolve-issue2459_-fall-back-to-writing-the-file-if-createlink-fails.dpatch
• unnamed

Setting milestone to 2.10.1 because these are important bug fixes.

These patches depend on a few sweeping patches that will not make it
into 2.10 ( "Drop GHC 7.4 support" above all).  I'd rather use my manual
port (see http://bugs.darcs.net/patch1380 ,
http://bugs.darcs.net/msg18629 ).

I've reviewed the "refactored Darcs.Repository" patch. A welcome cleanup!

Hi Ben,

Apologies for the delay in reviewing this (as always :-()

On 23/06/2015 00:45, Ben Franksen wrote:

> I am still unsure what the intention of the original code was w.r.t. the
> starting of two parallel threads. I am certain I did not misrepresent
> what the code actually did do, namely starting two threads that race for
> completion, abandoning (now: canceling) the non-yet-complete one. But is
> this really what should happen? One thread fetches all the patches the
> function gets as a parameter. The other thread fetches the patches
> listed in "meta-filelist-inventories". Are the passed patches garuanteed
> to be a subset of what's listed in meta-filelist-inventories? (This is
> absolutely not obvious and a small comment explaing the author's
> intention when writing this rather tricky code would have made things so
> much easier to understand!) Another question is why the thread that
> fetches the passed patches is started only when the meta-filelist has
> been written and not earlier. I suspect this was an artefact of the old
> implementation and could be changed now.

As far as I understand the old code, the pattern was to make one or more
calls to 'forkWithControlMVar' within the scope of a single
'withControlMVar' call:

> withControlMVar :: (MVar () -> IO ()) -> IO ()
> withControlMVar f = do
>   mv <- newMVar ()
>   f mv
>   takeMVar mv

> forkWithControlMVar :: MVar () -> IO () -> IO ()
> forkWithControlMVar mv f = do
>   takeMVar mv
>   _ <- forkIO $ finally f (putMVar mv ())
>   return ()

Don't the MVars ensure that the individual actions passed to
'forkWithControlMVar' are run in serial, and that the 'withControlMVar'
doesn't finish until the final forked action has completed? Initially
the MVar is full. When the first forkWithControlMVar runs, the MVar is
taken and the action is forked off. The second forkWithControlMVar will
have to wait for the first action to finish and call putMVar, before it
can do anything.

As I understand it your replacement code will run the actions in
parallel and then cancel the others once one completes.

As far as I can tell, in practice the number of actions that are run
depends on whether the code was in the 'basic' or the 'patches' call
path, and on how many entries were found in the tar file with a name
starting 'meta-'. I guess the expectation is that there would only be
one 'meta-' entry in the tar file, otherwise things could get a bit
confused. Under that assumption, there'd be one action in the 'basic'
call path used for reading 'pristine', and two actions in the 'patches'
call path used for reading 'inventories'.

Cheers,

Ganesh

I'm accepting the last past (use async) for the sake of having a
cleaner code and relying on a library that's designed for it. And I've
been cloning repos with this patch in screened for a few months now.

I wish we had right now more empirical evidence to back up this
change, but I'd rather accept this and take time to improve the repo
cloning code for 2.12.

I'm pasting Ganesh's comments that went on the mailing list only:

*****************

> patch 23bad6777ea545cbf8f5d2856ab3a50d190b0f8d
> Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
> Date:   Sun Jun 28 20:18:58 CEST 2015
>   * remove race from D.R.Packs, further simplify the code

Now I've got onto looking at this patch, I see my comments on the
previous patch may be a bit obsolete as things have moved on a bit and
now all the actions are all run to completion, now in parallel whereas
in the original code before your changes it was (I think) in serial.

One question about the followup is that now it doesn't specifically know
about the "meta-filelist-inventories" and "meta-filelist-pristine"
names. Does this matter?

I'm still rather confused by the whole module in general, I'll keep
trying to understand the intentions...

Cheers,

Ganesh

*****************

I found this specification: http://darcs.net/Internals/OptimizeHTTP

In particular the main goal of the parallel execution seems to be to
directly download files that are actually in the tarball, but in reverse
order. Even if this does lead to a wall clock speedup, I'm not sure it's
the best approach overall - it must cost bandwidth as well as adding a
lot of the complexity we see in the module.

We do need to download any patches explicitly requested that aren't in
the pack though. I'm not quite sure what the right strategy there is. If
there was a meta-filelist for the patches in the tarball we could read
that quickly (at the start of the download) and then start a parallel
thread to download patches that we know won't be in the tarball, but I
don't think there is.

I found this specification: http://darcs.net/Internals/OptimizeHTTP

In particular the main goal of the parallel execution seems to be to
directly download files that are actually in the tarball, but in reverse
order. Even if this does lead to a wall clock speedup, I'm not sure it's
the best approach overall - it must cost bandwidth as well as adding a
lot of the complexity we see in the module.

We do need to download any patches explicitly requested that aren't in
the pack though. I'm not quite sure what the right strategy there is. If
there was a meta-filelist for the patches in the tarball we could read
that quickly (at the start of the download) and then start a parallel
thread to download patches that we know won't be in the tarball, but I
don't think there is.

Ganesh

*****************

About the patch "remove race from D.R.Packs, further simplify the code".

I agree with the general idea of the patch (from its long description).

But one change of the patch seems weird to me: it's
fetchFilesUsingCache, as I discovered while debugging. It always
downloads all patches of the remote repo, while the previous version
stopped in the middle (I'm talking about the call to
fetchFilesUsingCache in the second thread, the one that gets patches one
by one).

So the idea of the patch is to no longer "meet in the middle", ie stop
everything when both threads meet? Thinking about it, this is indeed
more robust as it stays correct no matter what is the order inside of
the patches pack.

Another change I'm not sure about, is the call to `fetchFilesUsingCache`
inside of unpackTar. I would completely remove it in fact... and stop
using the meta- files altogether. (No only for patches but also pristine
pack). Even reading http://bugs.darcs.net/patch294 ,  I don't get the
motivation for them. Let's keep things simple (and even assume an evil
darcs client would create patches pack in any order).

Now about the implementation of packs (before Ben's patches) it seems
the implementation is backwards on one point.

I see newset2FL is used instead of "newset2RL" to build the patch hash
list we use to grab patches one by one. This means the list starts with
the oldest patches! I think it should be newset2RL. Also in
Darcs.UI.Commands.Optimize, the tarball is created from a newsetRL, ie
most recent patch first. I think this should also be inverted: pack
should have oldest-to-newest hashes, and we should manually download
newest-to-oldest patches. This would solve the case of packs not having
the newest patches of the repository (ie, packs being outdated, a common
case).

So, before accepting Ben's patch:

* can we ignore the meta- files altogether?
* invert patch sequence orders in one-by-one-getting and patches packs
(FL to RL and vice versa)

Here's a big followup to Ben's patch.

I'm not screening this new bundle yet but I wanted to share
my current progress on this. I've tested the code in various
configurations (including getting outdated patches pack).

All tests pass.

It does not implement http://bugs.darcs.net/issue2476 but
it leaves the clone code in a simpler shape, IMO.

4 patches for repository http://darcs.net:

patch 918fe9451c36701fb5fd5f8767444d990c2a3518
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Thu Oct 22 21:52:14 ART 2015
  * refactor clone code
  * take lock only once for eveything that happens after
    new repository initialization
  * factorize cache and sources handling
  * no longer look for new patches when getting basic pack,
    since we only get up-to-date basic packs anyway
  * show packs exceptions on standard output so that we fix
    them by the next release
  * switch patches retrieval order when using packs
    (makes more sense to have oldest patches in pack since
    it can be outdated)

patch a2a1a5b45c636915344a9d27096bddb86beec3f1
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Thu Oct 22 22:01:12 ART 2015
  * comment in doOptimizeHTTP

patch 3927bb3b8454d47065bd4f2598f127171a8d1fa3
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Thu Oct 22 22:21:09 ART 2015
  * ignore meta- files in packs when cloning
  Let's not try to be clever.

patch e9844d36ab99227fa9e69aa05d6a08f9c689f907
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Thu Oct 22 22:46:18 ART 2015
  * download patches pack asynchronously

Attachments

• patch-preview.txt
• refactor-clone-code.dpatch
• unnamed

I changed a few comments and added a few debug messages
so that it's possible to see what happens when cloning with
packs.

You can test from the following repositories:

-- this one has up to date packs in the current new order
-- (older to newer)
http://www.cs.famaf.unc.edu.ar/~hoffmann/darcs.net2/ 

-- this one has outdated packs (1 patch is missing) in the
-- old order  (newer to older)
http://www.cs.famaf.unc.edu.ar/~hoffmann/darcs.net2/ 

For instance:

    $ darcs clone http://www.cs.famaf.unc.edu.ar/~hoffmann/darcs.net2/  --verbose --packs  --debug 2>&1  |less

And then grep for TAR and FILE.

Performance is good, except when reading patch packs that
are in the old order, in which case pack grabbing stops
early and individual patch download continues.

I'm screening the bundle.

4 patches for repository http://darcs.net:

patch a2a1a5b45c636915344a9d27096bddb86beec3f1
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Thu Oct 22 22:01:12 ART 2015
  * comment in doOptimizeHTTP

patch 3927bb3b8454d47065bd4f2598f127171a8d1fa3
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Thu Oct 22 22:21:09 ART 2015
  * ignore meta- files in packs when cloning
  Let's not try to be clever.

patch 3ed524742966fe6a86e9836d30af63dd231b07c7
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Fri Oct 23 14:51:44 ART 2015
  * download patches pack asynchronously

patch 0ca671d83d1c4fd416889b5d073c9edb5bfd1e70
Author: Guillaume Hoffmann <guillaumh@gmail.com>
Date:   Fri Oct 23 15:30:20 ART 2015
  * refactor clone code
  * take lock only once for eveything that happens after
    new repository initialization
  * factorize cache and sources handling
  * no longer look for new patches when getting basic pack,
    since we only get up-to-date basic packs anyway
  * show packs exceptions on standard output so that we fix
    them by the next release
  * switch patches retrieval order when using packs
    (makes more sense to have oldest patches in pack since
    it can be outdated)

Attachments

• comment-in-dooptimizehttp.dpatch
• patch-preview.txt
• unnamed

Sorry, the repo with outdated packs and old order is:
http://www.cs.famaf.unc.edu.ar/~hoffmann/darcs.net/

I'm tempted to manually port the following change to branch 2.10 for the
2.10.2 release:

 * switch patches retrieval order when using packs
    (makes more sense to have oldest patches in pack since
    it can be outdated)

So that people sticking to the 2.10 versions on server will create packs
that will be more efficient for 2.12.

opinions?

To be more precise, the change I'd like to port is very short and in two
parts:

* make that optimize http put older patches first in the pack
* make that the "manual" thread of clone --packs get newer patches first

It's a matter of changing one newset2FL and mapFL into newset2RL and
mapRL in the Optimize module, and the other way around in the function
that gets complete repos with packs.

I'm attaching the patch I propose for branch 2.10.

Attachments

• switch-patches-retrieval-order-when-using-packs.dpatch

I'm fine with this change.

One possible thing to check is if it performs ok on large repos, as 
newset2FL traverses the repository in the "wrong" order? I'd guess that 
the darcs repository would be enough of a testcase there.

Before and after this change, calling newset2FL happens all the time,
the difference is that before, it happens at cloning time, while after
it happens only at "optimize http" time. Which is better because
optimizing should happen fewer times than cloning, and servers are
supposed to be more powerful than clients.

The only thing that is done with the FL created with newset2FL is that
we get the hashes of the individual patches of the FL, which is a cheap
operation (nothing like commuting and such). One test I did was running
"optimize http" twice on darcs.net's repo. The second time was much
faster, so I concluded that most of the time was spent reading files on
the disk.

Self-accepting the bundle into HEAD and the last single patch for
branch-2.10.