bug#67536: 29.1; Calc mode's math-read-preprocess-string conses unnecessarily

["Mattias Engdegård" <mattias.engdegard@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

2 dec. 2023 kl. 09.03 skrev Eli Zaretskii <eliz@gnu.org>:

> Mattias, any comments, or should I install this?

Well, the patch doesn't look too unreasonable so installing it would leave us better off than before.

Of course we may want to try to do better if this is really a bottleneck. The big job was to detect and locate the inefficiency -- thank you, Raffael! -- so his efforts were instrumental in any case.

There are minor points that could be addressed: `mapc` is often better replaced with `dolist`; the first position of a buffer is 1, not 0; and perhaps iterating through all elements of math-read-replacement-list isn't ideal.

Here's a variant which computes a single regexp to do the job and should cons a bit less.
Raffael, maybe you could see if this makes a difference in Org table performance.


[-- Attachment #2: math-read-preprocess-string.diff --]
[-- Type: application/octet-stream, Size: 2353 bytes --]

diff --git a/lisp/calc/calc-aent.el b/lisp/calc/calc-aent.el
index 66ede3295ae..4c1c464512a 100644
--- a/lisp/calc/calc-aent.el
+++ b/lisp/calc/calc-aent.el
@@ -547,22 +547,36 @@ math-read-subscripts
   "₀₁₂₃₄₅₆₇₈₉₊₋₍₎" ; 0123456789+-()
   "A string consisting of the subscripts allowed by Calc.")
 
+(defvar math--read-preprocess-re-cache nil
+  "Cached regexp and tag: (REGEXP REPLACEMENTS SUPERSCRIPTS SUBSCRIPTS)")
+
 ;;;###autoload
 (defun math-read-preprocess-string (str)
   "Replace some substrings of STR by Calc equivalents."
-  (setq str
-        (replace-regexp-in-string (concat "[" math-read-superscripts "]+")
-                                  "^(\\&)" str))
-  (setq str
-        (replace-regexp-in-string (concat "[" math-read-subscripts "]+")
-                                  "_(\\&)" str))
-  (let ((rep-list math-read-replacement-list))
-    (while rep-list
-      (setq str
-            (replace-regexp-in-string (nth 0 (car rep-list))
-                                      (nth 1 (car rep-list)) str))
-      (setq rep-list (cdr rep-list))))
-  str)
+  (unless (and (eq (nth 1 math--read-preprocess-re-cache)
+                   math-read-replacement-list)
+               (eq (nth 2 math--read-preprocess-re-cache)
+                   math-read-superscripts)
+               (eq (nth 3 math--read-preprocess-re-cache)
+                   math-read-subscripts))
+    ;; Cache invalid, recompute.
+    (setq math--read-preprocess-re-cache
+          (list (rx-to-string
+                 `(or (group (in ,math-read-superscripts))
+                      (group (in ,math-read-subscripts))
+                      (group (or ,@(mapcar #'car math-read-replacement-list))))
+                 t)
+                math-read-replacement-list
+                math-read-superscripts
+                math-read-subscripts)))
+  (replace-regexp-in-string
+   (nth 0 math--read-preprocess-re-cache)
+   (lambda (s)
+     (let ((r (cadr (assoc s math-read-replacement-list))))
+       (cond ((match-beginning 1) (concat "^(" r ")"))
+             ((match-beginning 2) (concat "_(" r ")"))
+             (t r))))
+   str t))
 
 ;; The next few variables are local to math-read-exprs (and math-read-expr
 ;; in calc-ext.el), but are set in functions they call.