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