[darcs-users] [patch257] Resolve issue 1503

Petr Rockai me at mornfall.net
Wed Jun 2 15:55:55 UTC 2010


Hi,

Eric Kow <kowey at darcs.net> writes:
> OK, I think I'm almost ready to apply this (no obvious bugs or things to
> complain about).
>>  copyFileUsingCache :: OrOnlySpeculate -> Cache -> HashedDir -> String -> IO ()
>> -copyFileUsingCache oos (Ca cache) subdir f =
>> -    do debugMessage $ "I'm doing copyFileUsingCache on "++(hashedDir subdir)++"/"++f
>> +copyFileUsingCache oos (Ca unsortedCache) subdir f =
>> +    do let (Ca cache) = Ca (sortBy compareByLocality unsortedCache)
>> +       debugMessage $ "I'm doing copyFileUsingCache on "++(hashedDir subdir)++"/"++f
>
> Interesting.  One question: does this sorting belong here?  What if in
> the future we want to sort by proximity to the repository directory?
> How would we manage that?  What's the right attitude to adopt here?  One
> possible answer, for example, is YAGNI.  Is that the right one?

I wouldn't say that future-proofing is the problem with this bit of
code. What I find sorely inadequate is that the caches are re-sorted for
every single file that needs to be fetched. Note that with populated
cache and hardlinking, this is a *very* fast operation and the constant
sorting could actually contribute non-negligible amount of time to it.

I think a more global solution that sorts the cache as it is populated
would be more appropriate.

>>  fetchFileUsingCachePrivate :: FromWhere -> Cache -> HashedDir -> String -> IO (String, B.ByteString)
>> -fetchFileUsingCachePrivate fromWhere (Ca cache) subdir f =
>> +fetchFileUsingCachePrivate fromWhere (Ca unsortedCache) subdir f =
>>      do when (fromWhere == Anywhere) $ copyFileUsingCache ActuallyCopy (Ca cache) subdir f
>>         ffuc cache
>>      `catchall` debugFail ("Couldn't fetch `"++f++"'\nin subdir "++(hashedDir subdir)++
>> hunk ./src/Darcs/Repository/Cache.hs 265
>>  
>>            ffuc [] = debugFail $ "No sources from which to fetch file `"++f++"'\n"++ show (Ca cache)
>>  
>> +          Ca cache = Ca (sortBy compareByLocality unsortedCache)
>
> What's the difference between these two functions?

(This obviously points out the other problem with the "late" sorting:
you need to do it in every function that non-trivially uses the
cache... so it's not just performance, but also maintainability)

Yours,
    Petr.


More information about the darcs-users mailing list