bug#66364: 29.1.50; vc-hg-registered/state are slow on directories of large repos

bug#66364: 29.1.50; vc-hg-registered/state are slow on directories of large repos

[Spencer Baugh]

vc-hg-state (and vc-hg-registered, which calls vc-hg-state) are slow
when run on directories of large repos.  In fact they're O(N) in the
number of files in the repo.

This is because vc-hg-state-slow runs "hg status" on directories in a
mode which lists all the files in the directory, and then it parses that
list.  Which is pointlessly slow.

However, Hg (like git) does not actually track directories.  So in the
end vc-hg-state on a directory should always be returning 'unregistered,
which matches the existing behavior of vc-hg (vc-hg-registered always
returned nil for directories) and is much faster.  (It also matches what
vc-git does.)

A patch to do this will follow

bug#66364: 29.1.50; vc-hg-registered/state are slow on directories of large repos

[Spencer Baugh]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Optimize-vc-hg-state-for-directories.patch --]
[-- Type: text/x-patch, Size: 1314 bytes --]

From 5573b678f816f81623deb7ebde66dfd3ebe92355 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Thu, 5 Oct 2023 11:35:25 -0400
Subject: [PATCH] Optimize vc-hg-state for directories

Directories are never tracked in hg, so it's pointless to run
vc-hg-state on them.  And, in fact, our implementation previously
would list all the files contained in the directory and then parse
that in Emacs, which is very slow in large repos.

Let's just use the knowledge that directories aren't tracked in hg,
and skip running hg entirely.

* lisp/vc/vc-hg.el (vc-hg-state): Return unregistered for directories.
---
 lisp/vc/vc-hg.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index c3e563a1f10..9a30706f519 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -216,8 +216,10 @@ vc-hg-registered
 
 (defun vc-hg-state (file)
   "Hg-specific version of `vc-state'."
-  (let ((state (vc-hg-state-fast file)))
-    (if (eq state 'unsupported) (vc-hg-state-slow file) state)))
+  (if (file-directory-p file)
+      'unregistered
+    (let ((state (vc-hg-state-fast file)))
+      (if (eq state 'unsupported) (vc-hg-state-slow file) state))))
 
 (defun vc-hg-state-slow (file)
   "Determine status of FILE by running hg."
-- 
2.39.3

bug#66364: 29.1.50; vc-hg-registered/state are slow on directories of large repos

[Dmitry Gutov]

On 05/10/2023 18:36, Spencer Baugh wrote:
>  From 5573b678f816f81623deb7ebde66dfd3ebe92355 Mon Sep 17 00:00:00 2001
> From: Spencer Baugh<sbaugh@janestreet.com>
> Date: Thu, 5 Oct 2023 11:35:25 -0400
> Subject: [PATCH] Optimize vc-hg-state for directories
> 
> Directories are never tracked in hg, so it's pointless to run
> vc-hg-state on them.  And, in fact, our implementation previously
> would list all the files contained in the directory and then parse
> that in Emacs, which is very slow in large repos.
> 
> Let's just use the knowledge that directories aren't tracked in hg,
> and skip running hg entirely.
> 
> * lisp/vc/vc-hg.el (vc-hg-state): Return unregistered for directories.
> ---
>   lisp/vc/vc-hg.el | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
> index c3e563a1f10..9a30706f519 100644
> --- a/lisp/vc/vc-hg.el
> +++ b/lisp/vc/vc-hg.el
> @@ -216,8 +216,10 @@ vc-hg-registered
>   
>   (defun vc-hg-state (file)
>     "Hg-specific version of `vc-state'."
> -  (let ((state (vc-hg-state-fast file)))
> -    (if (eq state 'unsupported) (vc-hg-state-slow file) state)))
> +  (if (file-directory-p file)
> +      'unregistered
> +    (let ((state (vc-hg-state-fast file)))
> +      (if (eq state 'unsupported) (vc-hg-state-slow file) state))))
>   
>   (defun vc-hg-state-slow (file)
>     "Determine status of FILE by running hg."

Perhaps we should just follow the example of vc-git-registered and 
return nil.

Could you mention which code calls 'registered' on a directory, though? 
If it's in-tree, that's probably a bug too.

bug#66364: 29.1.50; vc-hg-registered/state are slow on directories of large repos

[Spencer Baugh]

Dmitry Gutov <dmitry@gutov.dev> writes:
> On 05/10/2023 18:36, Spencer Baugh wrote:
>>  From 5573b678f816f81623deb7ebde66dfd3ebe92355 Mon Sep 17 00:00:00 2001
>> From: Spencer Baugh<sbaugh@janestreet.com>
>> Date: Thu, 5 Oct 2023 11:35:25 -0400
>> Subject: [PATCH] Optimize vc-hg-state for directories
>> Directories are never tracked in hg, so it's pointless to run
>> vc-hg-state on them.  And, in fact, our implementation previously
>> would list all the files contained in the directory and then parse
>> that in Emacs, which is very slow in large repos.
>> Let's just use the knowledge that directories aren't tracked in hg,
>> and skip running hg entirely.
>> * lisp/vc/vc-hg.el (vc-hg-state): Return unregistered for
>> directories.
>> ---
>>   lisp/vc/vc-hg.el | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>> diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
>> index c3e563a1f10..9a30706f519 100644
>> --- a/lisp/vc/vc-hg.el
>> +++ b/lisp/vc/vc-hg.el
>> @@ -216,8 +216,10 @@ vc-hg-registered
>>     (defun vc-hg-state (file)
>>     "Hg-specific version of `vc-state'."
>> -  (let ((state (vc-hg-state-fast file)))
>> -    (if (eq state 'unsupported) (vc-hg-state-slow file) state)))
>> +  (if (file-directory-p file)
>> +      'unregistered
>> +    (let ((state (vc-hg-state-fast file)))
>> +      (if (eq state 'unsupported) (vc-hg-state-slow file) state))))
>>     (defun vc-hg-state-slow (file)
>>     "Determine status of FILE by running hg."
>
> Perhaps we should just follow the example of vc-git-registered and
> return nil.

Also fine by me.

> Could you mention which code calls 'registered' on a directory,
> though? If it's in-tree, that's probably a bug too.

vc-root-diff.  Here's the trace:

* vc-hg-registered("~/test-hg-repos/empty/")
  apply(vc-hg-registered "~/test-hg-repos/empty/")
  vc-call-backend(Hg registered "~/test-hg-repos/empty/")
  #f(compiled-function (b) #<bytecode -0x10c889a4506dd5b1>)(Hg)
  mapc(#f(compiled-function (b) #<bytecode -0x10c889a4506dd5b1>) (FE RCS CVS SVN SCCS SRC Bzr Git Hg))
  vc-registered("~/test-hg-repos/empty/")
  vc-backend("~/test-hg-repos/empty/")
  vc-working-revision("~/test-hg-repos/empty/")
  vc-root-diff(nil t)
  funcall-interactively(vc-root-diff nil t)
  call-interactively(vc-root-diff nil nil)
  command-execute(vc-root-diff)

bug#66364: 29.1.50; vc-hg-registered/state are slow on directories of large repos

[Dmitry Gutov]

On 05/10/2023 19:19, Spencer Baugh wrote:
>> Could you mention which code calls 'registered' on a directory,
>> though? If it's in-tree, that's probably a bug too.
> vc-root-diff.  Here's the trace:
> 
> * vc-hg-registered("~/test-hg-repos/empty/")
>    apply(vc-hg-registered "~/test-hg-repos/empty/")
>    vc-call-backend(Hg registered "~/test-hg-repos/empty/")
>    #f(compiled-function (b) #<bytecode -0x10c889a4506dd5b1>)(Hg)
>    mapc(#f(compiled-function (b) #<bytecode -0x10c889a4506dd5b1>) (FE RCS CVS SVN SCCS SRC Bzr Git Hg))
>    vc-registered("~/test-hg-repos/empty/")
>    vc-backend("~/test-hg-repos/empty/")
>    vc-working-revision("~/test-hg-repos/empty/")
>    vc-root-diff(nil t)
>    funcall-interactively(vc-root-diff nil t)
>    call-interactively(vc-root-diff nil nil)
>    command-execute(vc-root-diff)

Huh, it actually looks like that call is unnecessary (the result is 
unused). These lines just come from 2009, mostly unchanged:

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index d3e53858c16..e3b1f7fafda 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2086,7 +2086,7 @@ vc-root-diff
      (vc-maybe-buffer-sync not-urgent)
      (let ((backend (vc-deduce-backend))
  	  (default-directory default-directory)
-	  rootdir working-revision)
+	  rootdir)
        (if backend
  	  (setq rootdir (vc-call-backend backend 'root default-directory))
  	(setq rootdir (read-directory-name "Directory for VC root-diff: "))
@@ -2094,14 +2094,13 @@ vc-root-diff
  	(if backend
  	    (setq default-directory rootdir)
  	  (error "Directory is not version controlled")))
-      (setq working-revision (vc-working-revision rootdir))
        ;; relative to it.  Bind default-directory to the root directory
        ;; here, this way the *vc-diff* buffer is setup correctly, so
        ;; relative file names work.
        (let ((default-directory rootdir))
          (vc-diff-internal
-         t (list backend (list rootdir) working-revision) nil nil
+         t (list backend (list rootdir)) nil nil
           (called-interactively-p 'interactive))))))

  ;;;###autoload

bug#66364: 29.1.50; vc-hg-registered/state are slow on directories of large repos

[sbaugh]

[-- Attachment #1: Type: text/plain, Size: 2429 bytes --]

Dmitry Gutov <dmitry@gutov.dev> writes:
> On 05/10/2023 19:19, Spencer Baugh wrote:
>>> Could you mention which code calls 'registered' on a directory,
>>> though? If it's in-tree, that's probably a bug too.
>> vc-root-diff.  Here's the trace:
>> * vc-hg-registered("~/test-hg-repos/empty/")
>>    apply(vc-hg-registered "~/test-hg-repos/empty/")
>>    vc-call-backend(Hg registered "~/test-hg-repos/empty/")
>>    #f(compiled-function (b) #<bytecode -0x10c889a4506dd5b1>)(Hg)
>>    mapc(#f(compiled-function (b) #<bytecode -0x10c889a4506dd5b1>) (FE RCS CVS SVN SCCS SRC Bzr Git Hg))
>>    vc-registered("~/test-hg-repos/empty/")
>>    vc-backend("~/test-hg-repos/empty/")
>>    vc-working-revision("~/test-hg-repos/empty/")
>>    vc-root-diff(nil t)
>>    funcall-interactively(vc-root-diff nil t)
>>    call-interactively(vc-root-diff nil nil)
>>    command-execute(vc-root-diff)
>
> Huh, it actually looks like that call is unnecessary (the result is
> unused). These lines just come from 2009, mostly unchanged:
>
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index d3e53858c16..e3b1f7fafda 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -2086,7 +2086,7 @@ vc-root-diff
>      (vc-maybe-buffer-sync not-urgent)
>      (let ((backend (vc-deduce-backend))
>  	  (default-directory default-directory)
> -	  rootdir working-revision)
> +	  rootdir)
>        (if backend
>  	  (setq rootdir (vc-call-backend backend 'root default-directory))
>  	(setq rootdir (read-directory-name "Directory for VC root-diff: "))
> @@ -2094,14 +2094,13 @@ vc-root-diff
>  	(if backend
>  	    (setq default-directory rootdir)
>  	  (error "Directory is not version controlled")))
> -      (setq working-revision (vc-working-revision rootdir))
>        ;; relative to it.  Bind default-directory to the root directory
>        ;; here, this way the *vc-diff* buffer is setup correctly, so
>        ;; relative file names work.
>        (let ((default-directory rootdir))
>          (vc-diff-internal
> -         t (list backend (list rootdir) working-revision) nil nil
> +         t (list backend (list rootdir)) nil nil
>           (called-interactively-p 'interactive))))))
>
>  ;;;###autoload

Yes, I would be quite happy with deleting these unused lines.  Although
I expect it's still nice to have the same optimization that vc-git does.

BTW, here's a version of my patch which follows vc-git and just returns
nil for directories.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Optimize-vc-hg-state-for-directories.patch --]
[-- Type: text/x-patch, Size: 1292 bytes --]

From 39a453555811aad18add3272d2325c69785f89c0 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Thu, 12 Oct 2023 21:06:53 -0400
Subject: [PATCH] Optimize vc-hg-state for directories

Directories are never tracked in hg, so it's pointless to run
vc-hg-state on them.  And, in fact, our implementation previously
would list all the files contained in the directory and then parse
that in Emacs, which is very slow in large repos.

Let's just use the knowledge that directories aren't tracked in hg,
and skip running hg entirely.

* lisp/vc/vc-hg.el (vc-hg-state): Return nil for
directories.  (Bug#66364)
---
 lisp/vc/vc-hg.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index c3e563a1f10..f2ee9ef35e4 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -216,8 +216,9 @@ vc-hg-registered
 
 (defun vc-hg-state (file)
   "Hg-specific version of `vc-state'."
-  (let ((state (vc-hg-state-fast file)))
-    (if (eq state 'unsupported) (vc-hg-state-slow file) state)))
+  (unless (file-directory-p file)
+    (let ((state (vc-hg-state-fast file)))
+      (if (eq state 'unsupported) (vc-hg-state-slow file) state))))
 
 (defun vc-hg-state-slow (file)
   "Determine status of FILE by running hg."
-- 
2.41.0

bug#66364: 29.1.50; vc-hg-registered/state are slow on directories of large repos

[Dmitry Gutov]

On 13/10/2023 04:09, sbaugh@catern.com wrote:
> Yes, I would be quite happy with deleting these unused lines.  Although
> I expect it's still nice to have the same optimization that vc-git does.
> 
> BTW, here's a version of my patch which follows vc-git and just returns
> nil for directories.

Certainly.

Installed both patches, thank you, and closing.