bug#70216: 30.0.50; self-insert-command doesn't respect create-lockfiles

bug#70216: 30.0.50; self-insert-command doesn't respect create-lockfiles

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi all!

I'm digging more and more into the internals of Emacs these days, and
while tracing input lag performance i noticed that we don't respect the
create-lockfiles option, at least not fully.

The documentation states:
```
If the option `create-lockfiles' is nil, this does nothing.
```
However, the nothing is postponed until later, inside the lock_file
function. This means we won't cretae the lockfile, but we will get the
truename-buffer, verify visited file, etc, on my system amounting to
some time spent which I thought I opted out of.

Along with a patch that fixes this behavior I've added 4 screenshots of
graphs taken through intel_pt tracing on trigger of self-insert inside a
c file.

to reproduce what I do, from inside my emacs source code folder:

```
;; normal emacs built from source a couple of days ago
emacs -Q +644 src/filelock.c
emacs -Q -eval "(setq create-lockfiles nil)" +644 src/filelock.c

;; emacs with the provided patch
./src/emacs -Q +644 src/filelock.c
./src/emacs -Q -eval "(setq create-lockfiles nil)" +644 src/filelock.c
```

The first two screenshots show the current behavior without this patch,
where you can see that we still do some lock-file related computing even
though the setting is off. The last two show that we don't do it at all,
while preserving the vanilla operation when create-lockfiles = t.

The lock file related code can take up to a millisecond on my system,
but is usually in the range of 200-300 microseconds.

It seems there was some cleanup/changes around this behavior which i
believe caused this as a regression, from around 3 years ago. I _think_
this commit: 9ce6541ac9710933beca7f9944087fe4849d5ae9

Theo


[-- Attachment #2: emacs-Q.png --]
[-- Type: image/png, Size: 151501 bytes --]

[-- Attachment #3: emacs-Q-create-lockfiles-nil.png --]
[-- Type: image/png, Size: 158928 bytes --]

[-- Attachment #4: patched-emacs-Q.png --]
[-- Type: image/png, Size: 162906 bytes --]

[-- Attachment #5: patched-emacs-Q-create-lockfiles-nil.png --]
[-- Type: image/png, Size: 232791 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0001-Actually-do-nothing-when-create-lockfiles-is-nil.patch --]
[-- Type: text/x-diff, Size: 2045 bytes --]

From c491a7533198ba875f8cc7dc26322744964b6000 Mon Sep 17 00:00:00 2001
From: Theodor Thornhill <theo@thornhill.no>
Date: Fri, 5 Apr 2024 12:34:13 +0200
Subject: [PATCH] Actually do nothing when create-lockfiles is nil

* src/filelock.c(lock-file, lock-buffer): Wrap the logic of the code in
a check for create-lockfiles.
---
 src/filelock.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/src/filelock.c b/src/filelock.c
index 8c27b226900..aad4f5abab1 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -642,18 +642,22 @@ DEFUN ("lock-file", Flock_file, Slock_file, 1, 1, 0,
 If the option `create-lockfiles' is nil, this does nothing.  */)
   (Lisp_Object file)
 {
+  if (create_lockfiles)
+    {
 #ifndef MSDOS
-  CHECK_STRING (file);
 
-  /* If the file name has special constructs in it,
-     call the corresponding file name handler.  */
-  Lisp_Object handler;
-  handler = Ffind_file_name_handler (file, Qlock_file);
-  if (!NILP (handler))
-    return call2 (handler, Qlock_file, file);
+      CHECK_STRING (file);
+
+      /* If the file name has special constructs in it,
+	 call the corresponding file name handler.  */
+      Lisp_Object handler;
+      handler = Ffind_file_name_handler (file, Qlock_file);
+      if (!NILP (handler))
+	return call2 (handler, Qlock_file, file);
 
-  lock_file (file);
+      lock_file (file);
 #endif	/* MSDOS */
+    }
   return Qnil;
 }
 
@@ -691,13 +695,16 @@ DEFUN ("lock-buffer", Flock_buffer, Slock_buffer,
 If the option `create-lockfiles' is nil, this does nothing.  */)
   (Lisp_Object file)
 {
-  if (NILP (file))
-    file = BVAR (current_buffer, file_truename);
-  else
-    CHECK_STRING (file);
-  if (SAVE_MODIFF < MODIFF
-      && !NILP (file))
-    Flock_file (file);
+  if (create_lockfiles)
+    {
+      if (NILP (file))
+	file = BVAR (current_buffer, file_truename);
+      else
+	CHECK_STRING (file);
+      if (SAVE_MODIFF < MODIFF
+	  && !NILP (file))
+	Flock_file (file);
+    }
   return Qnil;
 }
 
-- 
2.40.1

bug#70216: 30.0.50; self-insert-command doesn't respect create-lockfiles

[Eli Zaretskii]

> Date: Fri, 05 Apr 2024 12:52:39 +0200
> From:  Theodor Thornhill via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I'm digging more and more into the internals of Emacs these days, and
> while tracing input lag performance i noticed that we don't respect the
> create-lockfiles option, at least not fully.
> 
> The documentation states:
> ```
> If the option `create-lockfiles' is nil, this does nothing.
> ```

I guess we need to fix the documentation, then.

> However, the nothing is postponed until later, inside the lock_file
> function. This means we won't cretae the lockfile, but we will get the
> truename-buffer, verify visited file, etc, on my system amounting to
> some time spent which I thought I opted out of.

This is on purpose.  We decided that create-lockfiles = nil is meant
to disable the creation of lock files, but it is not meant to disable
userlock--ask-user-about-supersession-threat, for example, which is an
important feature we don't want to lose just because the user doesn't
want to have lock files.  See bug#53207 for some discussion of this.
Bug#49507 is also relevant.

> Along with a patch that fixes this behavior I've added 4 screenshots of
> graphs taken through intel_pt tracing on trigger of self-insert inside a
> c file.

I don't think we want to install this patch, since we do want the
file-change detection, which is done as part of this code.

> The lock file related code can take up to a millisecond on my system,
> but is usually in the range of 200-300 microseconds.

Lock files and file-change detection are Emacs safety measures that
are important on any modern OS.  Disabling them because they eat up
CPU cycles is wrong, from where I stand.  That said, hundreds of
milliseconds for 2 calls to 'stat' sounds excessive to me, so please
tell more details and try to show the breakdown of this long time.

> It seems there was some cleanup/changes around this behavior which i
> believe caused this as a regression, from around 3 years ago. I _think_
> this commit: 9ce6541ac9710933beca7f9944087fe4849d5ae9

There were more changes there, see above.

bug#70216: 30.0.50; self-insert-command doesn't respect create-lockfiles

[Eli Zaretskii]

> From: Theodor Thornhill <theo@thornhill.no>
> Cc: 70216@debbugs.gnu.org
> Date: Sat, 06 Apr 2024 23:29:56 +0200
> 
> >> The documentation states:
> >> ```
> >> If the option `create-lockfiles' is nil, this does nothing.
> >> ```
> >
> > I guess we need to fix the documentation, then.
> >
> 
> Sure ;)

Done on the emacs-29 branch.

> > Lock files and file-change detection are Emacs safety measures that
> > are important on any modern OS.  Disabling them because they eat up
> > CPU cycles is wrong, from where I stand.  That said, hundreds of
> > milliseconds for 2 calls to 'stat' sounds excessive to me, so please
> > tell more details and try to show the breakdown of this long time.
> >
> 
> Sure! So it all starts in prepare_to_modify_buffer, where we run the
> verify_internal_modification. This is pretty fast, in the order of ~10
> microseconds. Next up is the Flock_file, which delegates to
> lock_file. Before jumping into lock_file we run Ffind_file_name_handler,
> which takes between 50 and 200 microseconds on my system, depending on
> general load, I assume. now we jump into lock_file, from which we inside
> Fverify_visited_file_modtime run Ffind_file_name_handler for good
> measure, for another 50-200microseconds.
> 
> This to me looks like _at least_ one too many calls to
> Ffind_file_name_handler causing almost half a millisecond on keypress,
> which sounds very excessive. 

The extra call is only when the file is local.  And losing 50 to 200
microseconds doesn't sound bad to me.

Also, we don't call lock-file on each keystroke, only on the first one
that makes the buffer modified when it was previously unmodified.  So
when you type several characters in sequence, the net slowdown will be
very small.

In any case, you originally said this took hundreds of milliseconds,
not hundreds of microseconds.  Was that a typo, and you really meant
microseconds?

> If you're interested, take this trace file and plop it into
> https://magic-trace.org and search for self_insert_command and move
> around with wasd.

Hmm... you just sent a 15MB file, uncompressed, to everyone who is
subscribed to bug-gnu-emacs...

bug#70216: 30.0.50; self-insert-command doesn't respect create-lockfiles

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/html, Size: 3974 bytes --]

bug#70216: 30.0.50; self-insert-command doesn't respect create-lockfiles

[Eli Zaretskii]

> Date: Sun, 07 Apr 2024 08:40:58 +0200
> From: Theodor Thornhill <theo@thornhill.no>
> Cc: 70216@debbugs.gnu.org
> 
> I think we can close this if you don't think this is worth it, at least the docs got fixed :)

Closing.