I've attached the updated patches. Ihor Radchenko <[email protected]> writes:
> Kenny Chen <[email protected]> writes: > >> These patches improve org-string-width performance (fewer allocations) >> and cache its results in org-table-align. The actual improvement you'll >> see depends on how many cells have the same text (for cache hits), but I >> expect ~2x improvement in the worst cases. I think there's still quite a >> lot of room for optimization here. > > Likely yes. Further work on this or other performance bottlenecks > (including table formula calculation) would be welcome. Including beyond > the hackathon. I plan to submit one more (small) patchset later for this. Happy to take a look at other critical bottlenecks if you can point me to them. Kenny
>From 3bd4b0d97e5322f6b6668d7f3b2c021843c92f2e Mon Sep 17 00:00:00 2001 From: Kenny Chen <[email protected]> Date: Sat, 22 Nov 2025 12:28:12 -0500 Subject: [PATCH 1/4] org-macs.el: Cache Emacs version check for `org-string-width' * lisp/org-macs.el (org-string-width): Cache the `version<' call for performance. (org-string-width--old-emacs): New variable to cache `version<' call. --- lisp/org-macs.el | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lisp/org-macs.el b/lisp/org-macs.el index c3be41d02..37808289b 100644 --- a/lisp/org-macs.el +++ b/lisp/org-macs.el @@ -1159,6 +1159,9 @@ delimiting S." ((= cursor end) 0) (t (string-width (substring s cursor end))))))) +(defvar org-string-width--old-emacs (version< emacs-version "28") + "When non-nil, use fallback behavior, primarily for old Emacs versions.") + (defun org--string-width-1 (string) "Return width of STRING when displayed in the current buffer. Unlike `string-width', this function takes into consideration @@ -1173,7 +1176,7 @@ Results may be off sometimes if it cannot handle a given Return width in pixels when PIXELS is non-nil. When PIXELS is nil, DEFAULT-FACE is the face used to calculate relative STRING width. When REFERENCE-FACE is nil, `default' face is used." - (if (and (version< emacs-version "28") (not pixels)) + (if (and org-string-width--old-emacs (not pixels)) ;; FIXME: Fallback to old limited version, because ;; `window-pixel-width' is buggy in older Emacs. (org--string-width-1 string) -- 2.52.0
>From 598e1d96c354c52007595c4f960d5313ec3412f2 Mon Sep 17 00:00:00 2001 From: Kenny Chen <[email protected]> Date: Sat, 22 Nov 2025 14:35:02 -0500 Subject: [PATCH 2/4] org-macs.el: Construct `buffer-invisibility-spec' directly * lisp/org-macs.el (org-string-width): Construct the `buffer-invisibility-spec' with only one iteration. --- lisp/org-macs.el | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/lisp/org-macs.el b/lisp/org-macs.el index 37808289b..374a3353f 100644 --- a/lisp/org-macs.el +++ b/lisp/org-macs.el @@ -1208,7 +1208,14 @@ STRING width. When REFERENCE-FACE is nil, `default' face is used." '(org-fold-drawer org-fold-block org-fold-outline)))) - (push el result))) + (push + ;; Consider ellipsis to have 0 width. + ;; It is what Emacs 28+ does, but we have + ;; to force it in earlier Emacs versions. + (if (and (consp el) (cdr el)) + (list (car el)) + el) + result))) result))) (current-char-property-alias-alist char-property-alias-alist)) (with-current-buffer (get-buffer-create " *Org string width*" t) @@ -1216,16 +1223,7 @@ STRING width. When REFERENCE-FACE is nil, `default' face is used." (setq-local line-prefix nil) (setq-local wrap-prefix nil) (setq-local buffer-invisibility-spec - (if (listp current-invisibility-spec) - (mapcar (lambda (el) - ;; Consider ellipsis to have 0 width. - ;; It is what Emacs 28+ does, but we have - ;; to force it in earlier Emacs versions. - (if (and (consp el) (cdr el)) - (list (car el)) - el)) - current-invisibility-spec) - current-invisibility-spec)) + current-invisibility-spec) (setq-local char-property-alias-alist current-char-property-alias-alist) (let (pixel-width symbol-width) -- 2.52.0
>From ee2bdea989e4db44d4bb3996bee236f58fe8dcd3 Mon Sep 17 00:00:00 2001 From: Kenny Chen <[email protected]> Date: Sat, 22 Nov 2025 15:46:19 -0500 Subject: [PATCH 3/4] org-table.el: Cache cell width when aligning tables * etc/ORG-NEWS (~org-table--align-field~ now takes an optional ~field-width~ argument): Announce new optional argument. * lisp/org-table.el (org-table-align): Cache `org-string-width' results and reuse when possible instead of calling it for every cell, twice. (org-table--align-field): Accept a new optional argument for the field width. --- etc/ORG-NEWS | 5 +++++ lisp/org-table.el | 33 ++++++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS index 9113e68ea..a95960022 100644 --- a/etc/ORG-NEWS +++ b/etc/ORG-NEWS @@ -585,6 +585,11 @@ accommodate even a single character of the headline, after accounting for spaces and the surrounding parentheses, it will omit the headline entirely and just show as much of the clock as fits under the limit. +*** ~org-table--align-field~ now takes an optional ~field-width~ argument + +For performance, if the field's width is already known, it can be +passed in as ~field-width~ instead of having it be calculated again. + ** Removed or renamed functions and variables *** ~org-cycle-display-inline-images~ is renamed to ~org-cycle-display-link-previews~ diff --git a/lisp/org-table.el b/lisp/org-table.el index 551e18e9f..261be1ec3 100644 --- a/lisp/org-table.el +++ b/lisp/org-table.el @@ -4376,11 +4376,12 @@ extension of the given file name, and finally on the variable (user-error "TABLE_EXPORT_FORMAT invalid"))))) ;;;###autoload -(defun org-table--align-field (field width align) +(defun org-table--align-field (field width align &optional field-width) "Format FIELD according to column WIDTH and alignment ALIGN. FIELD is a string. WIDTH is a number. ALIGN is either \"c\", -\"l\" or\"r\"." - (let* ((spaces (- width (org-string-width field nil 'org-table))) +\"l\" or\"r\". If FIELD-WIDTH is non-nil, then it's used as +FIELD's width. Otherwise, it's calculated." + (let* ((spaces (- width (or field-width (org-string-width field nil 'org-table)))) (prefix (pcase align ("l" "") ("r" (make-string spaces ?\s)) @@ -4409,7 +4410,15 @@ FIELD is a string. WIDTH is a number. ALIGN is either \"c\", (rows (remq 'hline table)) (widths nil) (alignments nil) - (columns-number 1)) + (columns-number 1) + (cell-width-cache (make-hash-table :test 'equal)) + (get-or-compute-cell-width + (lambda (cell) + (or (gethash cell cell-width-cache) + (puthash + cell + (org-string-width cell nil 'org-table) + cell-width-cache))))) (if (null rows) ;; Table contains only horizontal rules. Compute the ;; number of columns anyway, and choose an arbitrary width @@ -4429,7 +4438,7 @@ FIELD is a string. WIDTH is a number. ALIGN is either \"c\", (non-empty 0)) (dolist (row rows) (let ((cell (or (nth i row) ""))) - (setq max-width (max max-width (org-string-width cell nil 'org-table))) + (setq max-width (max max-width (funcall get-or-compute-cell-width cell))) (cond (fixed-align? nil) ((equal cell "") nil) ((string-match "\\`<\\([lrc]\\)[0-9]*>\\'" cell) @@ -4470,10 +4479,16 @@ FIELD is a string. WIDTH is a number. ALIGN is either \"c\", (append row (make-list offset ""))))) (mapconcat #'identity - (cl-mapcar #'org-table--align-field - fields - widths - alignments) + (cl-mapcar + (lambda (field width alignment) + (org-table--align-field + field + width + alignment + (funcall get-or-compute-cell-width field))) + fields + widths + alignments) "|"))) "|"))) (if (equal new previous) -- 2.52.0
>From 54acbf76830564749c6e25039d57c6602b0132ad Mon Sep 17 00:00:00 2001 From: Kenny Chen <[email protected]> Date: Sat, 22 Nov 2025 15:31:23 -0500 Subject: [PATCH 4/4] org-table.el: Cache invisibility spec for `org-string-width' * etc/ORG-NEWS (New function ~org-string-width-invisibility-spec~): Announce new function. (~org-string-width~ now takes an optional ~invisibility-spec~ argument): Announce new optional argument. * lisp/org-macs.el (org-string-width): Refactor invisibility spec construction into a separate function, and accept the spec as a new optional argument. (org-string-width-invisibility-spec): New function to construct the invisibility spec for ~org-string-width'. * lisp/org-table.el (org-table-align): Call `org-string-width-invisibility-spec' once and pass the spec to the `org-string-width' calls. --- etc/ORG-NEWS | 11 +++++++++ lisp/org-macs.el | 59 +++++++++++++++++++++++++---------------------- lisp/org-table.el | 3 ++- 3 files changed, 45 insertions(+), 28 deletions(-) diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS index a95960022..ff04c04b4 100644 --- a/etc/ORG-NEWS +++ b/etc/ORG-NEWS @@ -494,6 +494,11 @@ for source blocks where the appropriate major mode is unavailable. The new function is like ~org-gnus-no-new-news~, but always opens the link in other frame. +*** New function ~org-string-width-invisibility-spec~ + +The new function constructs an invisibility spec without folds and +ellipses, suitable for ~org-string-width~. This can be helpful for +performance if ~org-string-width~ is called multiple times. *** New command ~org-link-preview~ to preview Org links @@ -590,6 +595,12 @@ show as much of the clock as fits under the limit. For performance, if the field's width is already known, it can be passed in as ~field-width~ instead of having it be calculated again. +*** ~org-string-width~ now takes an optional ~invisibility-spec~ argument + +For performance, if the invisibility spec has been constructed, it can +be passed in as ~invisibility-spec~ instead of having it be +constructed again. + ** Removed or renamed functions and variables *** ~org-cycle-display-inline-images~ is renamed to ~org-cycle-display-link-previews~ diff --git a/lisp/org-macs.el b/lisp/org-macs.el index 374a3353f..e27bfa773 100644 --- a/lisp/org-macs.el +++ b/lisp/org-macs.el @@ -1171,11 +1171,40 @@ Results may be off sometimes if it cannot handle a given `display' value." (org--string-from-props string 'display 0 (length string))) -(defun org-string-width (string &optional pixels default-face) +(defun org-string-width-invisibility-spec () + "Return the invisibility spec of this buffer without folds and ellipses." + ;; We need to remove the folds to make sure that folded table + ;; alignment is not messed up. + (or (and (not (listp buffer-invisibility-spec)) + buffer-invisibility-spec) + (let (result) + (dolist (el buffer-invisibility-spec) + (unless (or (memq el + '(org-fold-drawer + org-fold-block + org-fold-outline)) + (and (listp el) + (memq (car el) + '(org-fold-drawer + org-fold-block + org-fold-outline)))) + (push + ;; Consider ellipsis to have 0 width. + ;; It is what Emacs 28+ does, but we have + ;; to force it in earlier Emacs versions. + (if (and (consp el) (cdr el)) + (list (car el)) + el) + result))) + result))) + +(defun org-string-width (string &optional pixels default-face invisibility-spec) "Return width of STRING when displayed in the current buffer. Return width in pixels when PIXELS is non-nil. When PIXELS is nil, DEFAULT-FACE is the face used to calculate relative -STRING width. When REFERENCE-FACE is nil, `default' face is used." +STRING width. When REFERENCE-FACE is nil, `default' face is used. +Use INVISIBILITY-SPEC when non-nil, otherwise construct one without +folds and ellipses." (if (and org-string-width--old-emacs (not pixels)) ;; FIXME: Fallback to old limited version, because ;; `window-pixel-width' is buggy in older Emacs. @@ -1192,31 +1221,7 @@ STRING width. When REFERENCE-FACE is nil, `default' face is used." ;; when PIXELS are requested though). (unless pixels (put-text-property 0 (length string) 'face (or default-face 'default) string)) - (let (;; We need to remove the folds to make sure that folded table - ;; alignment is not messed up. - (current-invisibility-spec - (or (and (not (listp buffer-invisibility-spec)) - buffer-invisibility-spec) - (let (result) - (dolist (el buffer-invisibility-spec) - (unless (or (memq el - '(org-fold-drawer - org-fold-block - org-fold-outline)) - (and (listp el) - (memq (car el) - '(org-fold-drawer - org-fold-block - org-fold-outline)))) - (push - ;; Consider ellipsis to have 0 width. - ;; It is what Emacs 28+ does, but we have - ;; to force it in earlier Emacs versions. - (if (and (consp el) (cdr el)) - (list (car el)) - el) - result))) - result))) + (let ((current-invisibility-spec (or invisibility-spec (org-string-width-invisibility-spec))) (current-char-property-alias-alist char-property-alias-alist)) (with-current-buffer (get-buffer-create " *Org string width*" t) (setq-local display-line-numbers nil) diff --git a/lisp/org-table.el b/lisp/org-table.el index 261be1ec3..6a6f56689 100644 --- a/lisp/org-table.el +++ b/lisp/org-table.el @@ -4411,13 +4411,14 @@ FIELD's width. Otherwise, it's calculated." (widths nil) (alignments nil) (columns-number 1) + (invisibility-spec (org-string-width-invisibility-spec)) (cell-width-cache (make-hash-table :test 'equal)) (get-or-compute-cell-width (lambda (cell) (or (gethash cell cell-width-cache) (puthash cell - (org-string-width cell nil 'org-table) + (org-string-width cell nil 'org-table invisibility-spec) cell-width-cache))))) (if (null rows) ;; Table contains only horizontal rules. Compute the -- 2.52.0
