Re: [PATCH] git-download: Speed up 'git-predicate'.

[ludo@gnu.org (Ludovic Courtès)]

Hi!

Christopher Baines <mail@cbaines.net> skribis:

> On 07/06/17 13:52, Ludovic Courtès wrote:
>> Christopher Baines <mail@cbaines.net> skribis:

[...]

>> Do you know how much the inode optimization vs. the tree structure
>> contributes to the performance.
>
> I've got some more data. I ran the test script for smart-answers, and
> let it complete this time:
>
>   real    97m21.291s
>   user    120m50.400s
>   sys     0m31.020s
>
> Just applying the inode optimization gives this result:
>
>   real    90m28.116s
>   user    100m44.784s
>   sys     0m18.524s
>
> I'm going to assume that using the tree structure for directories makes
> up the rest of the difference. This will vary between repositories
> though, I think smart answers has a unusually large directory to file ratio.

Interesting, thanks for benchmarking!

>> Likewise, the (map car …) call for ‘match’.  :-)
>
> I'm stuck on this bit though, in the latest patch it reads:
>
>   (list? (member top-directory (map car tree))
>
> The list? call is just to turn the list or #f returned by member to #t
> or #f. The (map car tree) converts the tree to a list of top level
> directories. This bit of code is used when the last directory in the
> input list has been reached (e.g. when checking for foo/bar/baz
> top-directory would be baz) so the last check to perform is to check if
> baz exists at the current level of the tree. Any suggestions on
> restructuring this?

Probably:

  (match tree
    (((heads . _) ...)
     (->bool (member top-directory heads))))

>>> +         (inodes-vhash   (alist->vhash
>>> +                          (map
>>> +                           (lambda (file)
>>> +                             (let ((stat
>>> +                                    (lstat (string-append directory "/" file))))
>>> +                               (cons (stat:ino stat) (stat:dev stat))))
>>> +                           files)))
>> 
>> I would call it ‘inodes’ simply.  Also, we could use ‘list->set’ from
>> (guix sets) here.
>
> I've made the inodes-vhash -> inodes rename, but I was undecided about
> using (guix sets), is there a reason you recommended it?

Because conceptually it’s a set: what we want to know is whether the set
contains a given inode/device pair.

I’ll comment on v2 of the patch.

Thank you,
Ludo’.