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