org-ql-select function could be faster by caching the buffers variable

[molok]

OS/platform

Linux

Emacs version and provenance

Emacs 29.4 compiled by myself

Org version and provenance

9.7 shipped with emacs

org-ql package version and provenance

v0.8.5 (shipped with doom-emacs)

Description

I've got a usecase where I call org-ql-select 500 times and I've noticed that the caching could be improved since the buffers variable is kind-of expensive to evaluate (unless buffers-or-files is already a list of buffers).

org-ql/org-ql.el

Lines 377 to 393 in b7d4856

	(-let* ((buffers (->> (cl-typecase buffers-or-files
	(null (list (current-buffer)))
	(function (funcall buffers-or-files))
	(list buffers-or-files)
	(otherwise (list buffers-or-files)))
	(--map (cl-etypecase it
	;; NOTE: This etypecase is essential to opening links safely,
	;; as it rejects, e.g. lambdas in the buffers-files argument.
	(buffer it)
	(string (or (find-buffer-visiting it)
	(when (file-readable-p it)
	;; It feels unintuitive that `find-file-noselect' returns
	;; a buffer if the filename doesn't exist.
	(find-file-noselect it))
	(display-warning 'org-ql-select (format "Can't open file: %s" it) :error)))))
	;; Ignore special/hidden buffers.
	(--remove (string-prefix-p " " (buffer-name it)))))

I suspect many uses of org-ql-select pass a string or (org-agenda-files) as the argument of buffers-or-files so caching in the org-ql-select might make sense, but I'm not sure if the obvious implementation will handle all the edge-cases.

On my machine 500 function calls to org-ql-select passing the output of (org-agenda-files) take 5.8s, when passing a list of buffers it takes 1.9s so the difference is quite significant.

Again, I could adapt my code to simply pass a list of buffers, but it seems to be a low-hanging performance improvement that might be beneficial for all callers.

Etc.

No response

[alphapapa]

I doubt that caching that would be appropriate, because caching means adding a new "can of worms" to the whole procedure: how would we know when to invalidate the cache? What would the cache even bypass? The validations you cited are there for good reason.

This seems like an obvious case where, if you are calling org-ql-select 500 times in rapid succession, you should keep the list of buffers in the calling code.

Are you sure that the bottleneck is in org-ql-select and not in org-agenda-files?

Also, please explain more about your use case; it seems unusual, and I wonder if there might be other ways to improve performance.

[molok]

I'm not yet very familiar with the emacs platform so I have no idea if there are drawbacks in having a cache with the path as key and the buffer as the value of a hash-table, I feel there might be some edge-case that might be very hard to handle.

This seems like an obvious case where, if you are calling org-ql-select 500 times in rapid succession, you should keep the list of buffers in the calling code.

Sure, that's what I'm doing in my code now. IMHO it isn't really obvious that passing a list of buffers is way better than passing a list of paths, but again, it is only an issue if you call the function many times like I do.

Are you sure that the bottleneck is in org-ql-select and not in org-agenda-files?

yes, I'm sure that the bottleneck is in org-ql-select, see the benchmarks.

(defun molok/files-to-buffers (files)
  (let ((buffers '()))
    (dolist (file files)
      (let ((buffer (find-buffer-visiting file)))
        (unless buffer
          (setq buffer (find-file-noselect file)))
        (push buffer buffers)))
    (nreverse buffers)))

(length (org-agenda-files))
;; 27

;; org-ql-disabled and org-ql-select argument is a list of buffers: 4.7s
(let* ((buffers (molok/files-to-buffers (org-agenda-files))))
  (benchmark-run
      (mapcar
        (lambda(x)
          (let* ((search-string "msgid:xxxx.eurprd02.prod.outlook.com")
                 (org-ql-cache (make-hash-table))
                 (result (org-ql-select buffers `(regexp ,search-string)
                           :action (lambda ()
                                    (substring-no-properties (or (org-get-todo-state) ""))))))
              result))
        (number-sequence 1 500))))

;; org-ql-cache enabled and org-ql-select argument is a list of paths: 4.5s
(let* ((buffers (org-agenda-files)))
  (benchmark-run
      (mapcar
        (lambda(x)
          (let* ((search-string "msgid:xxxx.eurprd02.prod.outlook.com")
                 (result (org-ql-select buffers `(regexp ,search-string)
                           :action (lambda ()
                                    (substring-no-properties (or (org-get-todo-state) ""))))))
              result))
        (number-sequence 1 500))))

;; org-ql-cache enabled and org-ql-select argument is a list of buffers: 1.5s
(let* ((buffers (molok/files-to-buffers (org-agenda-files))))
  (benchmark-run
      (mapcar
        (lambda(x)
          (let* ((search-string "msgid:xxxx.eurprd02.prod.outlook.com")
                 (result (org-ql-select buffers `(regexp ,search-string)
                           :action (lambda ()
                                    (substring-no-properties (or (org-get-todo-state) ""))))))
              result))
        (number-sequence 1 500))))

Also, please explain more about your use case; it seems unusual, and I wonder if there might be other ways to improve performance.

I use mu4e as my mail client and I have a custom info for each email (header) that searches org-agenda-files for headers containing a link to that email, if one is found, it show its todo-state (TODO/DONE/etc).

My inbox shows 500 emails per screen, so that's why I need it to run 500 times.

 (setq mu4e-header-info-custom
        '(
          (:agenda-state .
                   (:name "Agenda"
                    :shortname "Agenda"
                    :help "agenda state"
                    :function
                    (lambda (msg)
                      (or (car (molok/org-ql-search-todo-state (plist-get msg :message-id))) ""))))))

;; original (slow) code
  (defun molok/org-ql-search-todo-state (search-string)
    (org-ql-select ((org-agenda-files)
      `(regexp ,search-string)
      :action (lambda ()
                    (substring-no-properties (or (org-get-todo-state) "")))))

;; new (fast) code
(defun molok/files-to-buffers (files)
  (let ((buffers '()))
    (dolist (file files)
      (let ((buffer (find-buffer-visiting file)))
        (unless buffer
          (setq buffer (find-file-noselect file)))
        (push buffer buffers)))
    (nreverse buffers)))

(defun molok/files-to-buffers-memoized(files)
  "this function can be expensive to evaluate, sometimes it makes sense to memoize it"
  (molok/files-to-buffers files))

(require 'memoize)
(memoize 'molok/files-to-buffers-memoized "1 hour")

(defun molok/org-ql-search-todo-state (search-string)
    (org-ql-select (molok/files-to-buffers-memoized (org-agenda-files))
      `(regexp ,search-string)
      :action (lambda ()
                    (substring-no-properties (or (org-get-todo-state) "")))))