GNU bug report logs - #78341
[PATCH] * src/w32.c (is_symlink): Quick return for non accessible file

Previous Next

To reply to this bug, email your comments to 78341 AT debbugs.gnu.org.
There is no need to reopen the bug first.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Lin Sun <sunlin7 <at> hotmail.com>
To: "bug-gnu-emacs <at> gnu.org" <bug-gnu-emacs <at> gnu.org>
Subject: [PATCH] * src/w32.c (is_symlink): Quick return for non accessible file
Date: Sat, 10 May 2025 05:01:27 +0000

[Message part 1 (text/plain, inline)]

When Emacs on windows will query file attributes twice even the file not exists in the function (faccessat).

This patch will distinguish the non-exists-file and short return to avoid the unnecessary file querying. 

On my testing Windows env, the "emacs.exe -nw -q" will reduce the file querying count from ~2700 to ~450. Reducing the Spacemacs (352 packages) startup time from ~7.5seconds to ~6.6seconds. 

Please help review the patch. Thank you.

[0001-src-w32.c-is_symlink-Quick-return-for-non-accessible.patch (text/x-patch, attachment)]

Message #8 received at 78341 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lin Sun <sunlin7 <at> hotmail.com>
Cc: 78341 <at> debbugs.gnu.org
Subject: Re: bug#78341: [PATCH] * src/w32.c (is_symlink): Quick return for non
 accessible file
Date: Sat, 10 May 2025 10:15:23 +0300

> From: Lin Sun <sunlin7 <at> hotmail.com>
> Date: Sat, 10 May 2025 05:01:27 +0000
> 
> When Emacs on windows will query file attributes twice even the file not exists in the function (faccessat).

Please tell more: where is the code that queries file attributes, and
how and why is that code invoked twice in this scenario?

> This patch will distinguish the non-exists-file and short return to avoid the unnecessary file querying. 

I'm not yet sure this kind of change is correct, because the protocol
between chase_symlinks and its callers is via the returned file name,
not via errno (which some of its code branches don't even set).  And
in general, if chase_symlinks returns the original file name, it just
means the file is not a symlink, it doesn't mean it's okay to fail the
faccessat call in its entirety.

So more details of the situation are required to see whether and how
it is possible to slash the time without breaking the correctness of
the functionality.

Thanks.

Message #11 received at 78341 <at> debbugs.gnu.org (full text, mbox):

From: Lin Sun <sunlin7 <at> hotmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: "78341 <at> debbugs.gnu.org" <78341 <at> debbugs.gnu.org>
Subject: Re: bug#78341: [PATCH] * src/w32.c (is_symlink): Quick return for non
 accessible file
Date: Sat, 10 May 2025 16:22:21 +0000

From: Eli Zaretskii <eliz <at> gnu.org>
Sent: Saturday, May 10, 2025 07:15 AM
> Please tell more: where is the code that queries file attributes, and
> how and why is that code invoked twice in this scenario?

In the w32.c, the faccessat() has the call chains: chase_symlinks -> is_symlink -> GetFileAttributes(); 
after that faccessat() will call the GetFileAttributes() again anyway. 

4113 faccessat (int dirfd, const char * path, int mode, int flags)
4151       path = chase_symlinks (path); -> is_symlink -> GetFileAttributes
4158       attributes = GetFileAttributesW (path_w);


As the patch shows, it cleans the errno first, then call the chase_symlinks(), then the errno was set if first GetFileAttributes() failed; so we can check errno for the non-exists case and return immediately. 

> > This patch will distinguish the non-exists-file and short return to avoid the unnecessary file querying. 
> 
> I'm not yet sure this kind of change is correct, because the protocol
> between chase_symlinks and its callers is via the returned file name,
> not via errno (which some of its code branches don't even set).  And
> in general, if chase_symlinks returns the original file name, it just
> means the file is not a symlink, it doesn't mean it's okay to fail the
> faccessat call in its entirety.
> 
> So more details of the situation are required to see whether and how
> it is possible to slash the time without breaking the correctness of
> the functionality.

The faccessat() in w32.c is Emacs specific one to simulating the POSIX function on Windows.
And chase_symlinks() function is Emacs defined function, we can define its errno behavior. This is windows only, won't affect other system.

It's reduce near 50% of the file queries on Windows, speedups the Emacs obviously. 

Message #14 received at 78341 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lin Sun <sunlin7 <at> hotmail.com>
Cc: 78341 <at> debbugs.gnu.org
Subject: Re: bug#78341: [PATCH] * src/w32.c (is_symlink): Quick return for non
 accessible file
Date: Sat, 10 May 2025 19:44:23 +0300

> From: Lin Sun <sunlin7 <at> hotmail.com>
> CC: "78341 <at> debbugs.gnu.org" <78341 <at> debbugs.gnu.org>
> Date: Sat, 10 May 2025 16:22:21 +0000
> msip_labels:
> 
> From: Eli Zaretskii <eliz <at> gnu.org>
> Sent: Saturday, May 10, 2025 07:15 AM
> > Please tell more: where is the code that queries file attributes, and
> > how and why is that code invoked twice in this scenario?
> 
> In the w32.c, the faccessat() has the call chains: chase_symlinks -> is_symlink -> GetFileAttributes(); 
> after that faccessat() will call the GetFileAttributes() again anyway. 
> 
> 4113 faccessat (int dirfd, const char * path, int mode, int flags)
> 4151       path = chase_symlinks (path); -> is_symlink -> GetFileAttributes
> 4158       attributes = GetFileAttributesW (path_w);

If that is the problem, we could make a local change in faccessat that
call GetFileAttributesW before checking for symlinks, and then avoid
calling chase_symlinks if the file is not a symlink.  Such a change
will not change the behavior of faccessat wrt setting errno, and this
will not have any effect except avoiding an extra GetFileAttributesW
call.

> > I'm not yet sure this kind of change is correct, because the protocol
> > between chase_symlinks and its callers is via the returned file name,
> > not via errno (which some of its code branches don't even set).  And
> > in general, if chase_symlinks returns the original file name, it just
> > means the file is not a symlink, it doesn't mean it's okay to fail the
> > faccessat call in its entirety.
> > 
> > So more details of the situation are required to see whether and how
> > it is possible to slash the time without breaking the correctness of
> > the functionality.
> 
> The faccessat() in w32.c is Emacs specific one to simulating the POSIX function on Windows.

Yes, but your patch potentially changes the value of errno that
faccessat returns, and that could have consequences elsewhere.  So I
prefer to make changes without such global consequences.

> And chase_symlinks() function is Emacs defined function, we can define its errno behavior. This is windows only, won't affect other system.

The effects on Emacs on Windows are also important.

> It's reduce near 50% of the file queries on Windows, speedups the Emacs obviously. 

I'm not against speeding up file access, I'm against risking unrelated
breakage due to changes that have a broader effect than they should.
The errno values we return from our emulated Posix APIs are important,
because Emacs's logic in many cases depends on that.

Please wait a bit for me to come up with an alternative patch for you
to try.

Message #17 received at 78341 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: sunlin7 <at> hotmail.com
Cc: 78341 <at> debbugs.gnu.org
Subject: Re: bug#78341: [PATCH] * src/w32.c (is_symlink): Quick return for non
 accessible file
Date: Sun, 11 May 2025 10:28:22 +0300

> Cc: 78341 <at> debbugs.gnu.org
> Date: Sat, 10 May 2025 19:44:23 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> Please wait a bit for me to come up with an alternative patch for you
> to try.

Please try the alternative patch below and see if it provides the same
speedups as your original one.

diff --git a/src/w32.c b/src/w32.c
index d7bf173..de270c8 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -4108,6 +4108,69 @@ logon_network_drive (const char *path)
     }
 }
 
+/* Subroutine of faccessat.  Determines attributes of FILE (which is
+   assumed to be in UTF-8 and after map_w32_filename) as reported by
+   GetFileAttributes.  Returns -1 if it fails (meaning the file doesn't
+   exist or cannot be accessed by the current user), otherwise returns
+   the bitmap of file's attributes.  */
+static DWORD
+access_attrs (const char *file)
+{
+  DWORD attrs;
+
+  if (w32_unicode_filenames)
+    {
+      wchar_t file_w[MAX_PATH];
+
+      filename_to_utf16 (file, file_w);
+      attrs = GetFileAttributesW (file_w);
+    }
+  else
+    {
+      char file_a[MAX_PATH];
+
+      filename_to_ansi (file, file_a);
+      attrs = GetFileAttributesA (file_a);
+    }
+
+  if (attrs == -1)
+    {
+      DWORD w32err = GetLastError ();
+
+      switch (w32err)
+	{
+	case ERROR_INVALID_NAME:
+	case ERROR_BAD_PATHNAME:
+	  if (is_unc_volume (file))
+	    {
+	      attrs = unc_volume_file_attributes (file);
+	      if (attrs == -1)
+		{
+		  errno = EACCES;
+		  return -1;
+		}
+	      return attrs;
+	    }
+	  /* FALLTHROUGH */
+	  FALLTHROUGH;
+	case ERROR_FILE_NOT_FOUND:
+	case ERROR_PATH_NOT_FOUND:
+	case ERROR_INVALID_DRIVE:
+	case ERROR_NOT_READY:
+	case ERROR_BAD_NETPATH:
+	case ERROR_BAD_NET_NAME:
+	  errno = ENOENT;
+	  break;
+	default:
+	  errno = EACCES;
+	  break;
+	}
+      return -1;
+    }
+
+  return attrs;
+}
+
 /* Emulate faccessat(2).  */
 int
 faccessat (int dirfd, const char * path, int mode, int flags)
@@ -4142,65 +4205,26 @@ faccessat (int dirfd, const char * path, int mode, int flags)
   /* MSVCRT implementation of 'access' doesn't recognize D_OK, and its
      newer versions blow up when passed D_OK.  */
   path = map_w32_filename (path, NULL);
+
+  attributes = access_attrs (path);
+  if (attributes == -1)	/* PATH doesn't exist or is inaccessible */
+    return -1;
+
   /* If the last element of PATH is a symlink, we need to resolve it
      to get the attributes of its target file.  Note: any symlinks in
      PATH elements other than the last one are transparently resolved
      by GetFileAttributes below.  */
+  int not_a_symlink = ((attributes & FILE_ATTRIBUTE_REPARSE_POINT) == 0);
   if ((volume_info.flags & FILE_SUPPORTS_REPARSE_POINTS) != 0
-      && (flags & AT_SYMLINK_NOFOLLOW) == 0)
-    path = chase_symlinks (path);
-
-  if (w32_unicode_filenames)
-    {
-      wchar_t path_w[MAX_PATH];
-
-      filename_to_utf16 (path, path_w);
-      attributes = GetFileAttributesW (path_w);
-    }
-  else
-    {
-      char path_a[MAX_PATH];
-
-      filename_to_ansi (path, path_a);
-      attributes = GetFileAttributesA (path_a);
-    }
-
-  if (attributes == -1)
+      && (flags & AT_SYMLINK_NOFOLLOW) == 0
+      && !not_a_symlink)
     {
-      DWORD w32err = GetLastError ();
-
-      switch (w32err)
-	{
-	case ERROR_INVALID_NAME:
-	case ERROR_BAD_PATHNAME:
-	  if (is_unc_volume (path))
-	    {
-	      attributes = unc_volume_file_attributes (path);
-	      if (attributes == -1)
-		{
-		  errno = EACCES;
-		  return -1;
-		}
-	      goto check_attrs;
-	    }
-	  /* FALLTHROUGH */
-	  FALLTHROUGH;
-	case ERROR_FILE_NOT_FOUND:
-	case ERROR_PATH_NOT_FOUND:
-	case ERROR_INVALID_DRIVE:
-	case ERROR_NOT_READY:
-	case ERROR_BAD_NETPATH:
-	case ERROR_BAD_NET_NAME:
-	  errno = ENOENT;
-	  break;
-	default:
-	  errno = EACCES;
-	  break;
-	}
-      return -1;
+      path = chase_symlinks (path);
+      attributes = access_attrs (path);
+      if (attributes == -1)
+	return -1;
     }
 
- check_attrs:
   if ((mode & X_OK) != 0
       && !(is_exec (path) || (attributes & FILE_ATTRIBUTE_DIRECTORY) != 0))
     {

Message #20 received at 78341 <at> debbugs.gnu.org (full text, mbox):

From: Lin Sun <sunlin7 <at> hotmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: "78341 <at> debbugs.gnu.org" <78341 <at> debbugs.gnu.org>
Subject: Re: bug#78341: [PATCH] * src/w32.c (is_symlink): Quick return for non
 accessible file
Date: Sun, 11 May 2025 15:58:55 +0000

Hi Eli,

I tried your patch on my local, can confirmed no 2nd file api call for the non-exists file, same as my previous one.

I compiled the Emacs with your patch on Windows and start it with the Spacemacs (heavy distribution) to open org files, c files... Run it for more than 1 hours, no side effect appeared, everything is good for me.

Please feel free to merge the patch and close the ticket. 

Thank you!

Message #25 received at 78341-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lin Sun <sunlin7 <at> hotmail.com>
Cc: 78341-done <at> debbugs.gnu.org
Subject: Re: bug#78341: [PATCH] * src/w32.c (is_symlink): Quick return for non
 accessible file
Date: Sun, 11 May 2025 19:32:27 +0300

> From: Lin Sun <sunlin7 <at> hotmail.com>
> CC: "78341 <at> debbugs.gnu.org" <78341 <at> debbugs.gnu.org>
> Date: Sun, 11 May 2025 15:58:55 +0000
> msip_labels:
> 
> Hi Eli,
> 
> I tried your patch on my local, can confirmed no 2nd file api call for the non-exists file, same as my previous one.
> 
> I compiled the Emacs with your patch on Windows and start it with the Spacemacs (heavy distribution) to open org files, c files... Run it for more than 1 hours, no side effect appeared, everything is good for me.
> 
> Please feel free to merge the patch and close the ticket. 

Thanks for testing, installed on master, and closing the bug.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.