bug#72689: 31.0.50; Proposal to improve string-pixel-width

bug#72689: 31.0.50; Proposal to improve string-pixel-width

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

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

Hello,

The function string-pixel-width is essential for calculating pixel
dimensions, especially for UI components. And in this context this
function can be called very often when the display is refreshed.

I propose the attached patch to make string-pixel-width faster while
using less memory, as shown by the following results of a basic
benchmark run in emacs -Q to compare the current implementation and this
proposal:

;; Basic benchmark
(let* ((text1 (make-string 1000 ?x))
        (text2 (propertize text1 'line-prefix "XXXX ")))
   (list
    (string-pixel-width text1)
    (string-pixel-width text2)
    (progn (garbage-collect)
           (benchmark-run 10000 (string-pixel-width text1)))
    (progn (garbage-collect)
           (benchmark-run 10000 (string-pixel-width text2)))
    ;;(insert text "\n")
    ))

;; Result with current implementation (4 run):
(12000 12000 (1.854707611 17 0.120106147) (1.884129905 17 0.12258791599999996))

(12000 12000 (1.846544798 17 0.12243524500000003) (1.8822177530000002 17 0.12349287399999997))

(12000 12000 (1.851244125 17 0.12162041699999998) (1.860517709 17 0.12352999599999998))

(12000 12000 (1.8542218929999998 17 0.12164553900000001) (1.856302462 17 0.122891689))

;; Result with proposed implementation (4 run):
(12000 12000 (1.698974522 0 0.0) (1.727446 2 0.014782505999999973))

(12000 12000 (1.701800124 0 0.0) (1.728024111 2 0.014718454999999908))

(12000 12000 (1.6850850800000001 0 0.0) (1.732370238 2 0.014801913000000111))

(12000 12000 (1.7356390130000001 0 0.0) (1.7858915800000001 2 0.014816158000000135))

 From my observations, the new implementation is around 8% faster, and
trigger less GC.  When there is no line-prefix property to
remove, the new implementation doesn't trigger any GC after 10000 runs.
Otherwise, only 2 GC are triggered instead of 17.

Maybe this proposal might be of interest, or at least provide some ideas
for improvement.

Thanks


In GNU Emacs 31.0.50 (build 6, x86_64-pc-linux-gnu, GTK+ Version
  3.24.43, cairo version 1.18.0) of 2024-08-17
Repository revision: 40eecd594ac60f38b6729fd9cf3474a8b9d133b9
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12014000
System Description: Fedora Linux 40 (KDE Plasma)

Configured using:
  'configure --with-x-toolkit=gtk3 --with-cairo-xcb
  --with-native-compilation=no
  PKG_CONFIG_PATH=/usr/local/lib/pkgconfig:/usr/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
   value of $LC_TIME: fr_FR.utf8
   value of $LANG: fr_FR.UTF-8
   locale-coding-system: utf-8-unix


[-- Attachment #2: improve-string-pixel-width-V00.patch --]
[-- Type: text/x-patch, Size: 2913 bytes --]

2024-08-17  David Ponce  <da_vid@orange.fr>

	Tweak the implementation of string-pixel-width to run faster and
	use less memory.

	* subr-x.el (string--pixel-width-buffer): New variable and function.
	(string--pixel-width): Use it.  Prefer `remove-text-properties' to
	`propertize' to avoid creating a new string on each call.


diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 058c06bc5f6..1f564fa5628 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -336,6 +336,23 @@ named-let
       (cl-labels ((,name ,fargs . ,body)) #',name)
       . ,aargs)))
 
+(defvar string--pixel-width-buffer nil)
+
+(defsubst string--pixel-width-buffer ()
+  "Get internal buffer used to calculate the pixel width of a string."
+  ;; Keeping a work buffer around is more efficient than creating a
+  ;; new temporary buffer on each call to `string-pixel-width'.
+  (if (buffer-live-p string--pixel-width-buffer)
+      string--pixel-width-buffer
+    (with-current-buffer (get-buffer-create " *string-pixel-width*" t)
+      ;; If `display-line-numbers' is enabled in internal buffers
+      ;; (e.g. globally), it breaks width calculation (bug#59311).
+      ;; Disable line-prefix and wrap-prefix, for the same reason.
+      ;; Set all these variables only one time here: they
+      ;; automatically become buffer-local when set.
+      (setq display-line-numbers nil line-prefix nil wrap-prefix nil)
+      (setq string--pixel-width-buffer (current-buffer)))))
+
 ;;;###autoload
 (defun string-pixel-width (string &optional buffer)
   "Return the width of STRING in pixels.
@@ -344,22 +361,18 @@ string-pixel-width
   (declare (important-return-value t))
   (if (zerop (length string))
       0
-    ;; Keeping a work buffer around is more efficient than creating a
-    ;; new temporary buffer.
-    (with-current-buffer (get-buffer-create " *string-pixel-width*")
-      ;; If `display-line-numbers' is enabled in internal buffers
-      ;; (e.g. globally), it breaks width calculation (bug#59311)
-      (setq-local display-line-numbers nil)
-      (delete-region (point-min) (point-max))
-      ;; Disable line-prefix and wrap-prefix, for the same reason.
-      (setq line-prefix nil
-	    wrap-prefix nil)
+    (with-current-buffer (string--pixel-width-buffer)
+      (erase-buffer)
       (if buffer
           (setq-local face-remapping-alist
                       (with-current-buffer buffer
                         face-remapping-alist))
         (kill-local-variable 'face-remapping-alist))
-      (insert (propertize string 'line-prefix nil 'wrap-prefix nil))
+      (insert string)
+      ;; Prefer `remove-text-properties' to `propertize' to avoid
+      ;; creating a new string on each call.
+      (remove-text-properties
+       (point-min) (point-max) '(line-prefix nil wrap-prefix nil))
       (car (buffer-text-pixel-size nil nil t)))))
 
 ;;;###autoload

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Eli Zaretskii]

> Date: Sun, 18 Aug 2024 00:03:22 +0200
> From:  David Ponce via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> The function string-pixel-width is essential for calculating pixel
> dimensions, especially for UI components. And in this context this
> function can be called very often when the display is refreshed.
> 
> I propose the attached patch to make string-pixel-width faster while
> using less memory, as shown by the following results of a basic
> benchmark run in emacs -Q to compare the current implementation and this
> proposal:
> 
> ;; Basic benchmark
> (let* ((text1 (make-string 1000 ?x))
>         (text2 (propertize text1 'line-prefix "XXXX ")))
>    (list
>     (string-pixel-width text1)
>     (string-pixel-width text2)
>     (progn (garbage-collect)
>            (benchmark-run 10000 (string-pixel-width text1)))
>     (progn (garbage-collect)
>            (benchmark-run 10000 (string-pixel-width text2)))
>     ;;(insert text "\n")
>     ))
> 
> ;; Result with current implementation (4 run):
> (12000 12000 (1.854707611 17 0.120106147) (1.884129905 17 0.12258791599999996))
> 
> (12000 12000 (1.846544798 17 0.12243524500000003) (1.8822177530000002 17 0.12349287399999997))
> 
> (12000 12000 (1.851244125 17 0.12162041699999998) (1.860517709 17 0.12352999599999998))
> 
> (12000 12000 (1.8542218929999998 17 0.12164553900000001) (1.856302462 17 0.122891689))
> 
> ;; Result with proposed implementation (4 run):
> (12000 12000 (1.698974522 0 0.0) (1.727446 2 0.014782505999999973))
> 
> (12000 12000 (1.701800124 0 0.0) (1.728024111 2 0.014718454999999908))
> 
> (12000 12000 (1.6850850800000001 0 0.0) (1.732370238 2 0.014801913000000111))
> 
> (12000 12000 (1.7356390130000001 0 0.0) (1.7858915800000001 2 0.014816158000000135))
> 
>  From my observations, the new implementation is around 8% faster, and
> trigger less GC.  When there is no line-prefix property to
> remove, the new implementation doesn't trigger any GC after 10000 runs.
> Otherwise, only 2 GC are triggered instead of 17.
> 
> Maybe this proposal might be of interest, or at least provide some ideas
> for improvement.

Thanks.  The idea SGTM, but I think the implementation needs to cater
for the case where more than one execution thread performs this job
"in parallel" (however improbable this could sound), so we need to be
able to detect when this buffer is "busy".  The simplest way is to use
some boolean buffer-local variable, which will be set non-nil when the
function starts using the buffer and reset to nil when the function is
done with its job.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

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

On 18/08/2024 6:40 AM, Eli Zaretskii wrote:
>> Date: Sun, 18 Aug 2024 00:03:22 +0200
>> From:  David Ponce via "Bug reports for GNU Emacs,
>>   the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> The function string-pixel-width is essential for calculating pixel
>> dimensions, especially for UI components. And in this context this
>> function can be called very often when the display is refreshed.
>>
>> I propose the attached patch to make string-pixel-width faster while
>> using less memory, as shown by the following results of a basic
>> benchmark run in emacs -Q to compare the current implementation and this
>> proposal:
>>
>> ;; Basic benchmark
>> (let* ((text1 (make-string 1000 ?x))
>>          (text2 (propertize text1 'line-prefix "XXXX ")))
>>     (list
>>      (string-pixel-width text1)
>>      (string-pixel-width text2)
>>      (progn (garbage-collect)
>>             (benchmark-run 10000 (string-pixel-width text1)))
>>      (progn (garbage-collect)
>>             (benchmark-run 10000 (string-pixel-width text2)))
>>      ;;(insert text "\n")
>>      ))
>>
>> ;; Result with current implementation (4 run):
>> (12000 12000 (1.854707611 17 0.120106147) (1.884129905 17 0.12258791599999996))
>>
>> (12000 12000 (1.846544798 17 0.12243524500000003) (1.8822177530000002 17 0.12349287399999997))
>>
>> (12000 12000 (1.851244125 17 0.12162041699999998) (1.860517709 17 0.12352999599999998))
>>
>> (12000 12000 (1.8542218929999998 17 0.12164553900000001) (1.856302462 17 0.122891689))
>>
>> ;; Result with proposed implementation (4 run):
>> (12000 12000 (1.698974522 0 0.0) (1.727446 2 0.014782505999999973))
>>
>> (12000 12000 (1.701800124 0 0.0) (1.728024111 2 0.014718454999999908))
>>
>> (12000 12000 (1.6850850800000001 0 0.0) (1.732370238 2 0.014801913000000111))
>>
>> (12000 12000 (1.7356390130000001 0 0.0) (1.7858915800000001 2 0.014816158000000135))
>>
>>   From my observations, the new implementation is around 8% faster, and
>> trigger less GC.  When there is no line-prefix property to
>> remove, the new implementation doesn't trigger any GC after 10000 runs.
>> Otherwise, only 2 GC are triggered instead of 17.
>>
>> Maybe this proposal might be of interest, or at least provide some ideas
>> for improvement.
> 
> Thanks.  The idea SGTM, but I think the implementation needs to cater
> for the case where more than one execution thread performs this job
> "in parallel" (however improbable this could sound), so we need to be
> able to detect when this buffer is "busy".  The simplest way is to use
> some boolean buffer-local variable, which will be set non-nil when the
> function starts using the buffer and reset to nil when the function is
> done with its job.

Thanks.  Your point about "parallelism" is quite relevant.  However
I'm not sure I understand your suggestion to introduce a buffer-local
boolean variable.  In particular, what to do when the buffer-local
flag is set when entering the function?  Wait until the flag is
reset?  Create another buffer?

Perhaps for parallelism, using temporary buffers might be a better
approach?  But I have no idea how much stress would be put on Emacs if
thousands of temporary buffers were created/freed during a session?

Could you elaborate further?

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Eli Zaretskii]

> Date: Sun, 18 Aug 2024 08:05:04 +0200
> Cc: 72689@debbugs.gnu.org
> From: David Ponce <da_vid@orange.fr>
> 
> On 18/08/2024 6:40 AM, Eli Zaretskii wrote:
> >> Date: Sun, 18 Aug 2024 00:03:22 +0200
> >> From:  David Ponce via "Bug reports for GNU Emacs,
> >>   the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >>
> >> The function string-pixel-width is essential for calculating pixel
> >> dimensions, especially for UI components. And in this context this
> >> function can be called very often when the display is refreshed.
> >>
> >> I propose the attached patch to make string-pixel-width faster while
> >> using less memory, as shown by the following results of a basic
> >> benchmark run in emacs -Q to compare the current implementation and this
> >> proposal:
> >>
> >> ;; Basic benchmark
> >> (let* ((text1 (make-string 1000 ?x))
> >>          (text2 (propertize text1 'line-prefix "XXXX ")))
> >>     (list
> >>      (string-pixel-width text1)
> >>      (string-pixel-width text2)
> >>      (progn (garbage-collect)
> >>             (benchmark-run 10000 (string-pixel-width text1)))
> >>      (progn (garbage-collect)
> >>             (benchmark-run 10000 (string-pixel-width text2)))
> >>      ;;(insert text "\n")
> >>      ))
> >>
> >> ;; Result with current implementation (4 run):
> >> (12000 12000 (1.854707611 17 0.120106147) (1.884129905 17 0.12258791599999996))
> >>
> >> (12000 12000 (1.846544798 17 0.12243524500000003) (1.8822177530000002 17 0.12349287399999997))
> >>
> >> (12000 12000 (1.851244125 17 0.12162041699999998) (1.860517709 17 0.12352999599999998))
> >>
> >> (12000 12000 (1.8542218929999998 17 0.12164553900000001) (1.856302462 17 0.122891689))
> >>
> >> ;; Result with proposed implementation (4 run):
> >> (12000 12000 (1.698974522 0 0.0) (1.727446 2 0.014782505999999973))
> >>
> >> (12000 12000 (1.701800124 0 0.0) (1.728024111 2 0.014718454999999908))
> >>
> >> (12000 12000 (1.6850850800000001 0 0.0) (1.732370238 2 0.014801913000000111))
> >>
> >> (12000 12000 (1.7356390130000001 0 0.0) (1.7858915800000001 2 0.014816158000000135))
> >>
> >>   From my observations, the new implementation is around 8% faster, and
> >> trigger less GC.  When there is no line-prefix property to
> >> remove, the new implementation doesn't trigger any GC after 10000 runs.
> >> Otherwise, only 2 GC are triggered instead of 17.
> >>
> >> Maybe this proposal might be of interest, or at least provide some ideas
> >> for improvement.
> > 
> > Thanks.  The idea SGTM, but I think the implementation needs to cater
> > for the case where more than one execution thread performs this job
> > "in parallel" (however improbable this could sound), so we need to be
> > able to detect when this buffer is "busy".  The simplest way is to use
> > some boolean buffer-local variable, which will be set non-nil when the
> > function starts using the buffer and reset to nil when the function is
> > done with its job.
> 
> Thanks.  Your point about "parallelism" is quite relevant.  However
> I'm not sure I understand your suggestion to introduce a buffer-local
> boolean variable.  In particular, what to do when the buffer-local
> flag is set when entering the function?  Wait until the flag is
> reset?  Create another buffer?

The latter, I think.

> Perhaps for parallelism, using temporary buffers might be a better
> approach?  But I have no idea how much stress would be put on Emacs if
> thousands of temporary buffers were created/freed during a session?
> 
> Could you elaborate further?

I think you understood the issue, and just creating a new buffer if
the "normal" one is "taken" should be good enough.  I don't expect
this situation to be a frequent one, so creating too many buffers
should not be a danger.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

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

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

On 18/08/2024 11:15 AM, Eli Zaretskii wrote:
[...]> I think you understood the issue, and just creating a new buffer if
> the "normal" one is "taken" should be good enough.  I don't expect
> this situation to be a frequent one, so creating too many buffers
> should not be a danger.

Thanks!  Please, find attached a revised V01 patch that implements
your suggestion.  I finally opted to keep only one function,
because setting a few buffer-local variables on each call has not a
significant impact on performance.


[-- Attachment #2: improve-string-pixel-width-V01.patch --]
[-- Type: text/x-patch, Size: 3914 bytes --]

2024-08-19  David Ponce  <da_vid@orange.fr>

	Tweak the implementation of string-pixel-width to run faster and
	use less memory.  Also cater for the case where more than one
	execution thread calls the function (bug#72689).

	* subr-x.el (string--pixel-width-buffer): New variable.
	(string--pixel-width): Use it.  Create a new working buffer when
	parallel run is detected.  Prefer `remove-text-properties' to
	`propertize' to avoid creating a new string on each call.

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 058c06bc5f6..b688d179da4 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -336,6 +336,8 @@ named-let
       (cl-labels ((,name ,fargs . ,body)) #',name)
       . ,aargs)))
 
+(defvar string--pixel-width-buffer nil)
+
 ;;;###autoload
 (defun string-pixel-width (string &optional buffer)
   "Return the width of STRING in pixels.
@@ -346,21 +348,45 @@ string-pixel-width
       0
     ;; Keeping a work buffer around is more efficient than creating a
     ;; new temporary buffer.
-    (with-current-buffer (get-buffer-create " *string-pixel-width*")
-      ;; If `display-line-numbers' is enabled in internal buffers
-      ;; (e.g. globally), it breaks width calculation (bug#59311)
-      (setq-local display-line-numbers nil)
-      (delete-region (point-min) (point-max))
-      ;; Disable line-prefix and wrap-prefix, for the same reason.
-      (setq line-prefix nil
-	    wrap-prefix nil)
-      (if buffer
-          (setq-local face-remapping-alist
-                      (with-current-buffer buffer
-                        face-remapping-alist))
-        (kill-local-variable 'face-remapping-alist))
-      (insert (propertize string 'line-prefix nil 'wrap-prefix nil))
-      (car (buffer-text-pixel-size nil nil t)))))
+    (with-current-buffer
+        (or (if (buffer-live-p string--pixel-width-buffer)
+                ;; Check if work buffer is not used by
+                ;; another parallel run (bug#72689).  That
+                ;; is, if `string--pixel-width-buffer' has no
+                ;; buffer-local value, in which case the function
+                ;; `buffer-local-value' returns its global value,
+                ;; equal to the work buffer.  Otherwise, when the
+                ;; work buffer is "busy", the buffer-local value of
+                ;; `string--pixel-width-buffer' is nil.
+                (buffer-local-value 'string--pixel-width-buffer
+                                    string--pixel-width-buffer))
+            ;; Create a new work buffer if current is "busy".
+            (setq string--pixel-width-buffer
+                  (generate-new-buffer " *string-pixel-width*" t)))
+      ;; Mark buffer as "busy".
+      (setq-local string--pixel-width-buffer nil)
+      (unwind-protect
+          (progn
+            ;; If `display-line-numbers' is enabled in internal
+            ;; buffers (e.g. globally), it breaks width calculation
+            ;; (bug#59311).  Disable `line-prefix' and `wrap-prefix',
+            ;; for the same reason.
+            (setq display-line-numbers nil
+                  line-prefix nil wrap-prefix nil)
+            (if buffer
+                (setq-local face-remapping-alist
+                            (with-current-buffer buffer
+                              face-remapping-alist))
+              (kill-local-variable 'face-remapping-alist))
+            (erase-buffer)
+            (insert string)
+            ;; Prefer `remove-text-properties' to `propertize' to avoid
+            ;; creating a new string on each call.
+            (remove-text-properties
+             (point-min) (point-max) '(line-prefix nil wrap-prefix nil))
+            (car (buffer-text-pixel-size nil nil t)))
+        ;; Mark buffer as free to use.
+        (kill-local-variable 'string--pixel-width-buffer)))))
 
 ;;;###autoload
 (defun string-glyph-split (string)

bug#72689: 31.0.50; Proposal to improve string-pixel-width

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

On 18/08/2024 11:15 AM, Eli Zaretskii wrote:
[...]
>>> Thanks.  The idea SGTM, but I think the implementation needs to cater
>>> for the case where more than one execution thread performs this job
>>> "in parallel" (however improbable this could sound), so we need to be
>>> able to detect when this buffer is "busy".  The simplest way is to use
>>> some boolean buffer-local variable, which will be set non-nil when the
>>> function starts using the buffer and reset to nil when the function is
>>> done with its job.

I've been thinking more about the parallelism issue when a function
reuses a temporary buffer for its activity, and I wonder if we could
use a simple API like the one below to safely get an exclusive working
buffer without having to create a new one on each call?

;; ---------------------------------------------------------------
(defvar work-buffer--list nil)
(defvar work-buffer--lock (make-mutex))

(defsubst work-buffer--get ()
   "Get a work buffer."
   (let ((buffer (with-mutex work-buffer--lock
                   (pop work-buffer--list))))
     (if (buffer-live-p buffer)
         buffer
       (generate-new-buffer " *work*" t))))

(defsubst work-buffer--release (buffer)
   "Release work BUFFER."
   (if (buffer-live-p buffer)
       (with-current-buffer buffer
         ;; Flush BUFFER before making it available again, i.e. clear
         ;; its contents, remove all overlays and buffer-local
         ;; variables.  Is it enough to safely reuse the buffer?
         (erase-buffer)
         (delete-all-overlays)
         (let (change-major-mode-hook) (kill-all-local-variables t))
         ;; Make the buffer available again.
         (with-mutex work-buffer--lock
           (push buffer work-buffer--list)))))

;;;###autoload
(defmacro with-work-buffer (&rest body)
   "Create a work buffer, and evaluate BODY there like `progn'.
Like `with-temp-buffer', but reuse an already created temporary
buffer when possible, instead of creating a new one on each call."
   (declare (indent 0) (debug t))
   (let ((work-buffer (make-symbol "work-buffer")))
     `(let ((,work-buffer (work-buffer--get)))
        (with-current-buffer ,work-buffer
          (unwind-protect
	     (progn ,@body)
            (work-buffer--release ,work-buffer))))))
;; ---------------------------------------------------------------

Here is how string-pixel-width could be implemented to use the above
API:

(defun string-pixel-W (string &optional buffer)
   "Return the width of STRING in pixels.
If BUFFER is non-nil, use the face remappings from that buffer when
determining the width."
   (declare (important-return-value t))
   (if (zerop (length string))
       0
     ;; Keeping a work buffer around is more efficient than creating a
     ;; new temporary buffer.
     (with-work-buffer
       ;; If `display-line-numbers' is enabled in internal
       ;; buffers (e.g. globally), it breaks width calculation
       ;; (bug#59311).  Disable `line-prefix' and `wrap-prefix',
       ;; for the same reason.
       (setq display-line-numbers nil
             line-prefix nil wrap-prefix nil)
       (if buffer
           (setq-local face-remapping-alist
                       (with-current-buffer buffer
                         face-remapping-alist))
         (kill-local-variable 'face-remapping-alist))
       (erase-buffer)
       (insert string)
       ;; Prefer `remove-text-properties' to `propertize' to avoid
       ;; creating a new string on each call.
       (remove-text-properties
        (point-min) (point-max) '(line-prefix nil wrap-prefix nil))
       (car (buffer-text-pixel-size nil nil t)))))

;; Quick benchmarck
(let* ((text1 (make-string 1000 ?x))
        (text2 (propertize text1 'line-prefix "XXXX "))
        (runs 1000))
   (list
    (progn (garbage-collect)
           (benchmark-run runs (string-pixel-width text2)))
    (progn (garbage-collect)
           (benchmark-run runs (string-pixel-W text2)))
    ))

Compared to my previous proposal the quick benchmark above shows
similar results for both performance and memory usage, but the new
implementation is simpler, and the API might be useful in other
similar cases.

WDYT?

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Eli Zaretskii]

> Date: Tue, 20 Aug 2024 17:12:45 +0200
> Cc: 72689@debbugs.gnu.org
> From: David Ponce <da_vid@orange.fr>
> 
> On 18/08/2024 11:15 AM, Eli Zaretskii wrote:
> [...]
> >>> Thanks.  The idea SGTM, but I think the implementation needs to cater
> >>> for the case where more than one execution thread performs this job
> >>> "in parallel" (however improbable this could sound), so we need to be
> >>> able to detect when this buffer is "busy".  The simplest way is to use
> >>> some boolean buffer-local variable, which will be set non-nil when the
> >>> function starts using the buffer and reset to nil when the function is
> >>> done with its job.
> 
> I've been thinking more about the parallelism issue when a function
> reuses a temporary buffer for its activity, and I wonder if we could
> use a simple API like the one below to safely get an exclusive working
> buffer without having to create a new one on each call?

Thanks, but using a mutex is overkill: there could be no race between
two or more threads in this case in accessing the buffer-local
variable, because only one Lisp thread can be running at any given
time.  So the simpler method of testing the "busy" flag should be
sufficient.

> Compared to my previous proposal the quick benchmark above shows
> similar results for both performance and memory usage, but the new
> implementation is simpler, and the API might be useful in other
> similar cases.

Simpler implementation is OK, but I think it will be simpler yet if
you remove the mutex, which is not needed.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

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

On 21/08/2024 3:17 PM, Eli Zaretskii wrote:
[...]
>>
>> I've been thinking more about the parallelism issue when a function
>> reuses a temporary buffer for its activity, and I wonder if we could
>> use a simple API like the one below to safely get an exclusive working
>> buffer without having to create a new one on each call?
> 
> Thanks, but using a mutex is overkill: there could be no race between
> two or more threads in this case in accessing the buffer-local
> variable, because only one Lisp thread can be running at any given
> time.  So the simpler method of testing the "busy" flag should be
> sufficient.

I used a mutex to protect the global variable `work-buffer--list'
(which holds the list buffers available to be reused) against
concurrent accesses during the very simple update operations: pop an
available buffer, push a released buffer to make it available.

Of course, if you guarantee that only one Lisp thread can be running
at any given time, protecting `work-buffer--list' against concurrent
accesses is not necessary and mutex can be removed.

>> Compared to my previous proposal the quick benchmark above shows
>> similar results for both performance and memory usage, but the new
>> implementation is simpler, and the API might be useful in other
>> similar cases.
> 
> Simpler implementation is OK, but I think it will be simpler yet if
> you remove the mutex, which is not needed.

Here is the new version without mutex (no significant impact on
performance compared to the version with mutex):

(defvar work-buffer--list nil)

(defsubst work-buffer--get ()
   "Get a work buffer."
   (let ((buffer (pop work-buffer--list)))
     (if (buffer-live-p buffer)
         buffer
       (generate-new-buffer " *work*" t))))

(defsubst work-buffer--release (buffer)
   "Release work BUFFER."
   (if (buffer-live-p buffer)
       (with-current-buffer buffer
         ;; Flush BUFFER before making it available again, i.e. clear
         ;; its contents, remove all overlays and buffer-local
         ;; variables.  Is it enough to safely reuse the buffer?
         (erase-buffer)
         (delete-all-overlays)
         (let (change-major-mode-hook) (kill-all-local-variables t))
         ;; Make the buffer available again.
         (push buffer work-buffer--list))))

;;;###autoload
(defmacro with-work-buffer (&rest body)
   "Create a work buffer, and evaluate BODY there like `progn'.
Like `with-temp-buffer', but reuse an already created temporary
buffer when possible, instead of creating a new one on each call."
   (declare (indent 0) (debug t))
   (let ((work-buffer (make-symbol "work-buffer")))
     `(let ((,work-buffer (work-buffer--get)))
        (with-current-buffer ,work-buffer
          (unwind-protect
	     (progn ,@body)
            (work-buffer--release ,work-buffer))))))

;;; Apply the work-buffer API to `string-pixel-width'.
;;
(defun string-pixel-width (string &optional buffer)
   "Return the width of STRING in pixels.
If BUFFER is non-nil, use the face remappings from that buffer when
determining the width."
   (declare (important-return-value t))
   (if (zerop (length string))
       0
     ;; Keeping a work buffer around is more efficient than creating a
     ;; new temporary buffer.
     (with-work-buffer
       ;; If `display-line-numbers' is enabled in internal
       ;; buffers (e.g. globally), it breaks width calculation
       ;; (bug#59311).  Disable `line-prefix' and `wrap-prefix',
       ;; for the same reason.
       (setq display-line-numbers nil
             line-prefix nil wrap-prefix nil)
       (if buffer
           (setq-local face-remapping-alist
                       (with-current-buffer buffer
                         face-remapping-alist))
         (kill-local-variable 'face-remapping-alist))
       (erase-buffer)
       (insert string)
       ;; Prefer `remove-text-properties' to `propertize' to avoid
       ;; creating a new string on each call.
       (remove-text-properties
        (point-min) (point-max) '(line-prefix nil wrap-prefix nil))
       (car (buffer-text-pixel-size nil nil t)))))

Should I prepare a patch of subr-x.el to include both the proposed
`work-buffer' API, and an implementation of `string-pixel-width' using
it?

Thanks!

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Eli Zaretskii]

> Date: Wed, 21 Aug 2024 22:43:08 +0200
> Cc: 72689@debbugs.gnu.org
> From: David Ponce <da_vid@orange.fr>
> 
> On 21/08/2024 3:17 PM, Eli Zaretskii wrote:
> > Thanks, but using a mutex is overkill: there could be no race between
> > two or more threads in this case in accessing the buffer-local
> > variable, because only one Lisp thread can be running at any given
> > time.  So the simpler method of testing the "busy" flag should be
> > sufficient.
> 
> I used a mutex to protect the global variable `work-buffer--list'
> (which holds the list buffers available to be reused) against
> concurrent accesses during the very simple update operations: pop an
> available buffer, push a released buffer to make it available.
> 
> Of course, if you guarantee that only one Lisp thread can be running
> at any given time, protecting `work-buffer--list' against concurrent
> accesses is not necessary and mutex can be removed.

Yes, I think so.

> Here is the new version without mutex (no significant impact on
> performance compared to the version with mutex):

Thanks, LGTM.

> Should I prepare a patch of subr-x.el to include both the proposed
> `work-buffer' API, and an implementation of `string-pixel-width' using
> it?

Yes, please.  And the macro itself needs a NEWS entry, I think.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

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

On 22/08/2024 5:43 AM, Eli Zaretskii wrote:
>> Date: Wed, 21 Aug 2024 22:43:08 +0200
>> Cc: 72689@debbugs.gnu.org
>> From: David Ponce <da_vid@orange.fr>
>>
>> On 21/08/2024 3:17 PM, Eli Zaretskii wrote:
>>> Thanks, but using a mutex is overkill: there could be no race between
>>> two or more threads in this case in accessing the buffer-local
>>> variable, because only one Lisp thread can be running at any given
>>> time.  So the simpler method of testing the "busy" flag should be
>>> sufficient.
>>
>> I used a mutex to protect the global variable `work-buffer--list'
>> (which holds the list buffers available to be reused) against
>> concurrent accesses during the very simple update operations: pop an
>> available buffer, push a released buffer to make it available.
>>
>> Of course, if you guarantee that only one Lisp thread can be running
>> at any given time, protecting `work-buffer--list' against concurrent
>> accesses is not necessary and mutex can be removed.
> 
> Yes, I think so.
> 
>> Here is the new version without mutex (no significant impact on
>> performance compared to the version with mutex):
> 
> Thanks, LGTM.
> 
>> Should I prepare a patch of subr-x.el to include both the proposed
>> `work-buffer' API, and an implementation of `string-pixel-width' using
>> it?
> 
> Yes, please.  And the macro itself needs a NEWS entry, I think.

Thanks.  I will prepare a patch and a NEWS entry for the new
`with-work-buffer' macro ASAP.

Could you please review this last version of the work-buffer API,
which introduce the variable `work-buffer-limit' to limit the number
of work buffers?

Such a limit minimizes the risk of keeping a large number of work
buffers throughout the entire Emacs session when, for example,
`with-work-buffer' is called recursively, as illustrated by this very
poorly written function:

;; Example of a poorly written function that could produce a big number
;; of work buffers!
(defun bad-make-string (char &optional count)
   (or (natnump count) (setq count 0))
   (with-work-buffer
     (insert-char char)
     (if (> count 1)
         (insert (bad-make-string char (1- count))))
     (buffer-string)))

For now, `work-buffer-limit' is a simple variable that can be
let-bound to temporarily increase the limit if needed.  I'm not sure a
customizable user option is useful.  The default limit of 10 can also
be changed if you think it is not enough or on the contrary is too
much.

Below is the new code.  The benchmarks result are still similar.

(defvar work-buffer--list nil)
(defvar work-buffer-limit 10
   "Maximum number of reusable work buffers.
When this limit is exceeded, newly allocated work buffers are
automatically killed, which means that in a such case
`with-work-buffer' becomes equivalent to `with-temp-buffer'.")

(defsubst work-buffer--get ()
   "Get a work buffer."
   (let ((buffer (pop work-buffer--list)))
     (if (buffer-live-p buffer)
         buffer
       (generate-new-buffer " *work*" t))))

(defun work-buffer--release (buffer)
   "Release work BUFFER."
   (if (buffer-live-p buffer)
       (with-current-buffer buffer
         ;; Flush BUFFER before making it available again, i.e. clear
         ;; its contents, remove all overlays and buffer-local
         ;; variables.  Is it enough to safely reuse the buffer?
         (erase-buffer)
         (delete-all-overlays)
         (let (change-major-mode-hook)
	  (kill-all-local-variables t))
         ;; Make the buffer available again.
         (push buffer work-buffer--list)))
   ;; If the maximum number of reusable work buffers is exceeded, kill
   ;; work buffer in excess, taking into account that the limit could
   ;; have been let-bound to temporarily increase its value.
   (when (> (length work-buffer--list) work-buffer-limit)
     (mapc #'kill-buffer (nthcdr work-buffer-limit work-buffer--list))
     (setq work-buffer--list (ntake work-buffer-limit work-buffer--list))))

;;;###autoload
(defmacro with-work-buffer (&rest body)
   "Create a work buffer, and evaluate BODY there like `progn'.
Like `with-temp-buffer', but reuse an already created temporary
buffer when possible, instead of creating a new one on each call."
   (declare (indent 0) (debug t))
   (let ((work-buffer (make-symbol "work-buffer")))
     `(let ((,work-buffer (work-buffer--get)))
        (with-current-buffer ,work-buffer
          (unwind-protect
	     (progn ,@body)
            (work-buffer--release ,work-buffer))))))

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Eli Zaretskii]

> Date: Thu, 22 Aug 2024 11:48:48 +0200
> Cc: 72689@debbugs.gnu.org
> From: David Ponce <da_vid@orange.fr>
> 
> > Thanks, LGTM.
> > 
> >> Should I prepare a patch of subr-x.el to include both the proposed
> >> `work-buffer' API, and an implementation of `string-pixel-width' using
> >> it?
> > 
> > Yes, please.  And the macro itself needs a NEWS entry, I think.
> 
> Thanks.  I will prepare a patch and a NEWS entry for the new
> `with-work-buffer' macro ASAP.
> 
> Could you please review this last version of the work-buffer API,
> which introduce the variable `work-buffer-limit' to limit the number
> of work buffers?

LGTM, thanks.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

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

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

On 22/08/2024 12:59 PM, Eli Zaretskii wrote:
>> Date: Thu, 22 Aug 2024 11:48:48 +0200
>> Cc: 72689@debbugs.gnu.org
>> From: David Ponce <da_vid@orange.fr>
>>
>>> Thanks, LGTM.
>>>
>>>> Should I prepare a patch of subr-x.el to include both the proposed
>>>> `work-buffer' API, and an implementation of `string-pixel-width' using
>>>> it?
>>>
>>> Yes, please.  And the macro itself needs a NEWS entry, I think.
>>
>> Thanks.  I will prepare a patch and a NEWS entry for the new
>> `with-work-buffer' macro ASAP.
>>
>> Could you please review this last version of the work-buffer API,
>> which introduce the variable `work-buffer-limit' to limit the number
>> of work buffers?
> 
> LGTM, thanks.

Thanks.  Please find attached 2 patches:

1. with-work-buffer.patch to introduce the new macro with-work-buffer.

2. string-pixel-width.patch to improve `string-pixel-width'.

[-- Attachment #2: with-work-buffer.patch --]
[-- Type: text/x-patch, Size: 3382 bytes --]

2024-08-22  David Ponce  <da_vid@orange.fr>

	New macro `with-work-buffer'.

	* etc/NEWS: Announce 'with-work-buffer'.

	* lisp/emacs-lisp/subr-x.el (work-buffer--list)
	(work-buffer-limit): New variables.
	(work-buffer--get, work-buffer--release): New function.
	(with-work-buffer): New macro.

diff --git a/etc/NEWS b/etc/NEWS
index 8cd21f5fb74..d6440540b45 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -257,6 +257,14 @@ language A.
 If supplied, 'string-pixel-width' will use any face remappings from
 BUFFER when computing the string's width.
 
+---
+*** New macro 'with-work-buffer'.
+This macro is similar to the already existing macro `with-temp-buffer'.
+However, contrary to the latter, `with-work-buffer' does not allocate a
+new temporary buffer on each call, but try to reuse those previously
+allocated (up to a number defined by the variable `work-buffer-limit'
+which defaults to 10).
+
 +++
 ** 'date-to-time' now defaults to local time.
 The function now assumes local time instead of Universal Time when
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 058c06bc5f6..3347c802f68 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -336,6 +336,53 @@ named-let
       (cl-labels ((,name ,fargs . ,body)) #',name)
       . ,aargs)))
 
+(defvar work-buffer--list nil)
+(defvar work-buffer-limit 10
+  "Maximum number of reusable work buffers.
+When this limit is exceeded, newly allocated work buffers are
+automatically killed, which means that in a such case
+`with-work-buffer' becomes equivalent to `with-temp-buffer'.")
+
+(defsubst work-buffer--get ()
+  "Get a work buffer."
+  (let ((buffer (pop work-buffer--list)))
+    (if (buffer-live-p buffer)
+        buffer
+      (generate-new-buffer " *work*" t))))
+
+(defun work-buffer--release (buffer)
+  "Release work BUFFER."
+  (if (buffer-live-p buffer)
+      (with-current-buffer buffer
+        ;; Flush BUFFER before making it available again, i.e. clear
+        ;; its contents, remove all overlays and buffer-local
+        ;; variables.  Is it enough to safely reuse the buffer?
+        (erase-buffer)
+        (delete-all-overlays)
+        (let (change-major-mode-hook)
+          (kill-all-local-variables t))
+        ;; Make the buffer available again.
+        (push buffer work-buffer--list)))
+  ;; If the maximum number of reusable work buffers is exceeded, kill
+  ;; work buffer in excess, taking into account that the limit could
+  ;; have been let-bound to temporarily increase its value.
+  (when (> (length work-buffer--list) work-buffer-limit)
+    (mapc #'kill-buffer (nthcdr work-buffer-limit work-buffer--list))
+    (setq work-buffer--list (ntake work-buffer-limit work-buffer--list))))
+
+;;;###autoload
+(defmacro with-work-buffer (&rest body)
+  "Create a work buffer, and evaluate BODY there like `progn'.
+Like `with-temp-buffer', but reuse an already created temporary
+buffer when possible, instead of creating a new one on each call."
+  (declare (indent 0) (debug t))
+  (let ((work-buffer (make-symbol "work-buffer")))
+    `(let ((,work-buffer (work-buffer--get)))
+       (with-current-buffer ,work-buffer
+         (unwind-protect
+             (progn ,@body)
+           (work-buffer--release ,work-buffer))))))
+
 ;;;###autoload
 (defun string-pixel-width (string &optional buffer)
   "Return the width of STRING in pixels.

[-- Attachment #3: string-pixel-width.patch --]
[-- Type: text/x-patch, Size: 2054 bytes --]

2024-08-22  David Ponce  <da_vid@orange.fr>

	Tweak the implementation of string-pixel-width to run faster and
	use less memory.  Also cater for the case where this function is
	called in parallel (bug#72689).

	* lisp/emacs-lisp/subr-x.el (string-pixel-width): Use
	`with-work-macro'.  Prefer `remove-text-properties' to
	`propertize' to avoid creating a new string on each call.

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 3347c802f68..83528f5c5dd 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -393,20 +393,24 @@ string-pixel-width
       0
     ;; Keeping a work buffer around is more efficient than creating a
     ;; new temporary buffer.
-    (with-current-buffer (get-buffer-create " *string-pixel-width*")
-      ;; If `display-line-numbers' is enabled in internal buffers
-      ;; (e.g. globally), it breaks width calculation (bug#59311)
-      (setq-local display-line-numbers nil)
-      (delete-region (point-min) (point-max))
-      ;; Disable line-prefix and wrap-prefix, for the same reason.
-      (setq line-prefix nil
-	    wrap-prefix nil)
+    (with-work-buffer
+      ;; If `display-line-numbers' is enabled in internal
+      ;; buffers (e.g. globally), it breaks width calculation
+      ;; (bug#59311).  Disable `line-prefix' and `wrap-prefix',
+      ;; for the same reason.
+      (setq display-line-numbers nil
+            line-prefix nil wrap-prefix nil)
       (if buffer
           (setq-local face-remapping-alist
                       (with-current-buffer buffer
                         face-remapping-alist))
         (kill-local-variable 'face-remapping-alist))
-      (insert (propertize string 'line-prefix nil 'wrap-prefix nil))
+      (erase-buffer)
+      (insert string)
+      ;; Prefer `remove-text-properties' to `propertize' to avoid
+      ;; creating a new string on each call.
+      (remove-text-properties
+       (point-min) (point-max) '(line-prefix nil wrap-prefix nil))
       (car (buffer-text-pixel-size nil nil t)))))
 
 ;;;###autoload

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Juri Linkov]

> +*** New macro 'with-work-buffer'.
> +This macro is similar to the already existing macro `with-temp-buffer'.

Another variant for the name that emphasizes its opposite nature
to `with-temp-buffer' would be `with-permanent-buffer'
or `with-perm-buffer'.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  72689@debbugs.gnu.org
> Date: Fri, 23 Aug 2024 09:20:07 +0300
> 
> > +*** New macro 'with-work-buffer'.
> > +This macro is similar to the already existing macro `with-temp-buffer'.
> 
> Another variant for the name that emphasizes its opposite nature
> to `with-temp-buffer' would be `with-permanent-buffer'
> or `with-perm-buffer'.

FWIW, I like with-work-buffer better.  But that doesn't mean there
couldn't be proposals for better names.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

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

On 23/08/2024 8:49 AM, Eli Zaretskii wrote:
>> From: Juri Linkov <juri@linkov.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  72689@debbugs.gnu.org
>> Date: Fri, 23 Aug 2024 09:20:07 +0300
>>
>>> +*** New macro 'with-work-buffer'.
>>> +This macro is similar to the already existing macro `with-temp-buffer'.
>>
>> Another variant for the name that emphasizes its opposite nature
>> to `with-temp-buffer' would be `with-permanent-buffer'
>> or `with-perm-buffer'.
> 
> FWIW, I like with-work-buffer better.  But that doesn't mean there
> couldn't be proposals for better names.

Same for me.  The name `with-work-buffer' clearly stay that you are
using a work buffer, which is actually a temp buffer, excepted that
the implementation try to minimize the number of new allocated buffer.

`with-work-buffer' could be safely replaced by `with-temp-buffer' and
probably `with-temp-buffer' by `with-work-buffer' in many cases.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Eli Zaretskii]

> Date: Thu, 22 Aug 2024 16:56:11 +0200
> Cc: 72689@debbugs.gnu.org
> From: David Ponce <da_vid@orange.fr>
> 
> > LGTM, thanks.
> 
> Thanks.  Please find attached 2 patches:
> 
> 1. with-work-buffer.patch to introduce the new macro with-work-buffer.
> 
> 2. string-pixel-width.patch to improve `string-pixel-width'.

Thanks, installed on the master branch, and closing the bug.

Please in the future try to submit patches using "git format-patch",
as that makes the patches easier to apply.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

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

On 31/08/2024 10:26 AM, Eli Zaretskii wrote:
>> Date: Thu, 22 Aug 2024 16:56:11 +0200
>> Cc: 72689@debbugs.gnu.org
>> From: David Ponce <da_vid@orange.fr>
>>
>>> LGTM, thanks.
>>
>> Thanks.  Please find attached 2 patches:
>>
>> 1. with-work-buffer.patch to introduce the new macro with-work-buffer.
>>
>> 2. string-pixel-width.patch to improve `string-pixel-width'.
> 
> Thanks, installed on the master branch, and closing the bug.

Many thanks!

Looking at the new version of string-pixel-width, I realized that I left
a call to erase-buffer that is not necessary.  Leaving it is not a problem,
but it would save a function call to remove it.

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 83528f5c5dd..4cdb065feeb 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -405,7 +405,6 @@ string-pixel-width
                        (with-current-buffer buffer
                          face-remapping-alist))
          (kill-local-variable 'face-remapping-alist))
-      (erase-buffer)
        (insert string)
        ;; Prefer `remove-text-properties' to `propertize' to avoid
        ;; creating a new string on each call.

> 
> Please in the future try to submit patches using "git format-patch",
> as that makes the patches easier to apply.

Ok, I will have study this command.
I'm afraid I only use git in a very basic way.

Many thanks for your help!

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Eli Zaretskii]

> Date: Sat, 31 Aug 2024 12:51:52 +0200
> Cc: 72689-done@debbugs.gnu.org
> From: David Ponce <da_vid@orange.fr>
> 
> > Thanks, installed on the master branch, and closing the bug.
> 
> Many thanks!
> 
> Looking at the new version of string-pixel-width, I realized that I left
> a call to erase-buffer that is not necessary.  Leaving it is not a problem,
> but it would save a function call to remove it.

Thanks, done.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Jim Porter]

On 8/17/2024 3:03 PM, David Ponce via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> I propose the attached patch to make string-pixel-width faster while
> using less memory, as shown by the following results of a basic
> benchmark run in emacs -Q to compare the current implementation and this
> proposal:[snip]

> -      (insert (propertize string 'line-prefix nil 'wrap-prefix nil))
> +      (insert string)
> +      ;; Prefer `remove-text-properties' to `propertize' to avoid
> +      ;; creating a new string on each call.
> +      (remove-text-properties
> +       (point-min) (point-max) '(line-prefix nil wrap-prefix nil))

Is this change safe? I suppose most of the time, this wouldn't matter, 
but I could imagine a case where a caller wanted the pixel-width of a 
string that had one of those properties set, and they'd want those 
properties to be preserved (e.g. if the string was to be inserted into a 
buffer later).

Maybe this code could just check for the presence of 'line-prefix' or 
'wrap-prefix' properties and only call 'propertize' if they're in the 
string? (Or maybe it's even possible to leave the string as-is and 
compute the width in some way that accounts for these properties in the 
correct way...)

I'm not sure how much this matters, but I always get a bit nervous about 
a function changing something about one of its arguments when it's not 
obvious.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

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

On 18/08/2024 8:12 AM, Jim Porter wrote:
> On 8/17/2024 3:03 PM, David Ponce via Bug reports for GNU Emacs, the Swiss army knife of text editors wrote:
>> I propose the attached patch to make string-pixel-width faster while
>> using less memory, as shown by the following results of a basic
>> benchmark run in emacs -Q to compare the current implementation and this
>> proposal:[snip]
> 
>> -      (insert (propertize string 'line-prefix nil 'wrap-prefix nil))
>> +      (insert string)
>> +      ;; Prefer `remove-text-properties' to `propertize' to avoid
>> +      ;; creating a new string on each call.
>> +      (remove-text-properties
>> +       (point-min) (point-max) '(line-prefix nil wrap-prefix nil))
> 
> Is this change safe? I suppose most of the time, this wouldn't matter, but I could imagine a case where a caller wanted the pixel-width of a string that had one of those properties set, and they'd want those properties to be preserved (e.g. if the string was to be inserted into a buffer later).
> 
> Maybe this code could just check for the presence of 'line-prefix' or 'wrap-prefix' properties and only call 'propertize' if they're in the string? (Or maybe it's even possible to leave the string as-is and compute the width in some way that accounts for these properties in the correct way...)
> 
> I'm not sure how much this matters, but I always get a bit nervous about a function changing something about one of its arguments when it's not obvious.

My implementation proposal does not change the behavior of the
function. It is just an attempt to improve its performance.

As far as I am concerned I do not think that the values ​​of
line-prefix, wrap prefix and display-line-numbers should be taken into
account because the number of pixels of a string should not depend
on the position where this string is displayed, but only on its own
display attributes like faces and other display properties.

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Jim Porter]

On 8/18/2024 12:36 AM, David Ponce via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> On 18/08/2024 8:12 AM, Jim Porter wrote:
>> I'm not sure how much this matters, but I always get a bit nervous 
>> about a function changing something about one of its arguments when 
>> it's not obvious.
> 
> My implementation proposal does not change the behavior of the
> function. It is just an attempt to improve its performance.

My mistake; I misread the code and thought the 'remove-text-properties' 
call was being applied to the string argument. (It's applied to the 
buffer, of course.)

bug#72689: 31.0.50; Proposal to improve string-pixel-width

[Eli Zaretskii]

> Date: Sat, 17 Aug 2024 23:12:21 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 8/17/2024 3:03 PM, David Ponce via Bug reports for GNU Emacs, the 
> Swiss army knife of text editors wrote:
> > I propose the attached patch to make string-pixel-width faster while
> > using less memory, as shown by the following results of a basic
> > benchmark run in emacs -Q to compare the current implementation and this
> > proposal:[snip]
> 
> > -      (insert (propertize string 'line-prefix nil 'wrap-prefix nil))
> > +      (insert string)
> > +      ;; Prefer `remove-text-properties' to `propertize' to avoid
> > +      ;; creating a new string on each call.
> > +      (remove-text-properties
> > +       (point-min) (point-max) '(line-prefix nil wrap-prefix nil))
> 
> Is this change safe?

It's how this function behaved since day one.  These properties are
meaningless for a string's width, since for them to take effect the
string should be at the beginning of a screen line, which is not a
property of an arbitrary string.

> I suppose most of the time, this wouldn't matter, 
> but I could imagine a case where a caller wanted the pixel-width of a 
> string that had one of those properties set, and they'd want those 
> properties to be preserved (e.g. if the string was to be inserted into a 
> buffer later).

If a Lisp program wants to know the width of the line-prefix, it can
do that separately.

> I'm not sure how much this matters, but I always get a bit nervous about 
> a function changing something about one of its arguments when it's not 
> obvious.

The argument isn't changed, only the text inserted into the temporary
buffer gets its properties changed.