GNU bug report logs - #72689
31.0.50; Proposal to improve string-pixel-width

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 72689 in the body.
You can then email your comments to 72689 AT debbugs.gnu.org in the normal way.

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

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

From: David Ponce <da_vid <at> orange.fr>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50; Proposal to improve string-pixel-width
Date: Sun, 18 Aug 2024 00:03:22 +0200

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

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

[improve-string-pixel-width-V00.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: David Ponce <da_vid <at> orange.fr>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Sun, 18 Aug 2024 07:40:42 +0300

> 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 <at> 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.

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Sun, 18 Aug 2024 08:05:04 +0200

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 <at> 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?

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: David Ponce <da_vid <at> orange.fr>, 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Sat, 17 Aug 2024 23:12:21 -0700

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.

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

From: David Ponce <da_vid <at> orange.fr>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Sun, 18 Aug 2024 09:36:47 +0200

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.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: David Ponce <da_vid <at> orange.fr>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Sun, 18 Aug 2024 12:15:14 +0300

> Date: Sun, 18 Aug 2024 08:05:04 +0200
> Cc: 72689 <at> debbugs.gnu.org
> From: David Ponce <da_vid <at> 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 <at> 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.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: da_vid <at> orange.fr, 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Sun, 18 Aug 2024 12:23:14 +0300

> Date: Sat, 17 Aug 2024 23:12:21 -0700
> From: Jim Porter <jporterbugs <at> 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.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: David Ponce <da_vid <at> orange.fr>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Sun, 18 Aug 2024 09:35:28 -0700

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.)

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Mon, 19 Aug 2024 10:49:05 +0200

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

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.

[improve-string-pixel-width-V01.patch (text/x-patch, attachment)]

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Tue, 20 Aug 2024 17:12:45 +0200

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?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: David Ponce <da_vid <at> orange.fr>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Wed, 21 Aug 2024 16:17:12 +0300

> Date: Tue, 20 Aug 2024 17:12:45 +0200
> Cc: 72689 <at> debbugs.gnu.org
> From: David Ponce <da_vid <at> 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.

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Wed, 21 Aug 2024 22:43:08 +0200

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!

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: David Ponce <da_vid <at> orange.fr>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Thu, 22 Aug 2024 06:43:40 +0300

> Date: Wed, 21 Aug 2024 22:43:08 +0200
> Cc: 72689 <at> debbugs.gnu.org
> From: David Ponce <da_vid <at> 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.

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Thu, 22 Aug 2024 11:48:48 +0200

On 22/08/2024 5:43 AM, Eli Zaretskii wrote:
>> Date: Wed, 21 Aug 2024 22:43:08 +0200
>> Cc: 72689 <at> debbugs.gnu.org
>> From: David Ponce <da_vid <at> 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))))))

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: David Ponce <da_vid <at> orange.fr>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Thu, 22 Aug 2024 13:59:04 +0300

> Date: Thu, 22 Aug 2024 11:48:48 +0200
> Cc: 72689 <at> debbugs.gnu.org
> From: David Ponce <da_vid <at> 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.

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Thu, 22 Aug 2024 16:56:11 +0200

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

On 22/08/2024 12:59 PM, Eli Zaretskii wrote:
>> Date: Thu, 22 Aug 2024 11:48:48 +0200
>> Cc: 72689 <at> debbugs.gnu.org
>> From: David Ponce <da_vid <at> 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'.

[with-work-buffer.patch (text/x-patch, attachment)]

[string-pixel-width.patch (text/x-patch, attachment)]

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

From: Juri Linkov <juri <at> linkov.net>
To: David Ponce <da_vid <at> orange.fr>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
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'.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: da_vid <at> orange.fr, 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Fri, 23 Aug 2024 09:49:51 +0300

> From: Juri Linkov <juri <at> linkov.net>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  72689 <at> 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.

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>, Juri Linkov <juri <at> linkov.net>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Fri, 23 Aug 2024 09:23:26 +0200

On 23/08/2024 8:49 AM, Eli Zaretskii wrote:
>> From: Juri Linkov <juri <at> linkov.net>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>,  72689 <at> 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.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: David Ponce <da_vid <at> orange.fr>
Cc: 72689-done <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Sat, 31 Aug 2024 11:26:59 +0300

> Date: Thu, 22 Aug 2024 16:56:11 +0200
> Cc: 72689 <at> debbugs.gnu.org
> From: David Ponce <da_vid <at> 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.

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72689-done <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Sat, 31 Aug 2024 12:51:52 +0200

On 31/08/2024 10:26 AM, Eli Zaretskii wrote:
>> Date: Thu, 22 Aug 2024 16:56:11 +0200
>> Cc: 72689 <at> debbugs.gnu.org
>> From: David Ponce <da_vid <at> 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!

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: David Ponce <da_vid <at> orange.fr>
Cc: 72689 <at> debbugs.gnu.org
Subject: Re: bug#72689: 31.0.50; Proposal to improve string-pixel-width
Date: Sat, 31 Aug 2024 15:00:31 +0300

> Date: Sat, 31 Aug 2024 12:51:52 +0200
> Cc: 72689-done <at> debbugs.gnu.org
> From: David Ponce <da_vid <at> 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.

Previous Next

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