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" <bug-gnu-emacs@gnu.org>]

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