From 8d57a691257a3d0f292f549dee01ad5446ce94e3 Mon Sep 17 00:00:00 2001 From: Adam Porter Date: Fri, 30 Aug 2019 09:34:34 -0500 Subject: [PATCH] Change/Fix: (--select-cached) Query-cache key and narrowing 1. It seems that caching was broken in 3adaf4e5fcd676a020a589a6ba70b32fc17070d2, because I changed what key was used to retrieve from the cache but not the key that was used to store in the cache. I guess I hadn't noticed the performance difference. It should be fixed now. 2. While making this fix, the test suite caught a bug in the new, almost-fixed version that was caused by not including the preamble in the key, and it only did so because the tests ran sequentially in the same Emacs, making use of caching. If each test were independent and ran with a clean cache, it wouldn't have caught the bug, and it would undoubtedly have been quite a head-scratcher, or even a hair-puller, at some future time. 3. And I would have pushed a version including that bug if I didn't have a git pre-push hook that runs the tests, because somehow I missed running the tests on that particular change. So, lessons reinforced: tests are good; automated tests are better; enforced tests are best. 4. Narrowed queries are now cached using point-min/max in the buffer. This will be especially helpful for the WIP recursive queries feature. I do wonder if the overhead of caching might be a drawback in some cases, however some simple benchmarks of recursive queries that return about 2,000 results shows a large improvement from caching, reducing runtime from 3.12 to 0.24 seconds, so it's probably worth it, overall. --- org-ql.el | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/org-ql.el b/org-ql.el index 885b7f5..5392cdf 100644 --- a/org-ql.el +++ b/org-ql.el @@ -540,15 +540,20 @@ replace the clause with a preamble." "Return results for ARGS and current buffer using cache." ;; MAYBE: Timeout cached queries. Probably not necessarily since they will be removed when a ;; buffer is closed, or when a query is run after modifying a buffer. - ;; FIXME: Narrowed queries will probably conflict in the cache, because the region is not - ;; stored. We should either not cache narrow queries, or store the region with it. - (-let (((&plist :query query :action action :narrow narrow) args)) + (-let* (((&plist :query :preamble-re :action :narrow) args) + (query-cache-key + ;; The key must include the preamble, because some queries are replaced by + ;; the preamble, leaving a nil query, which would make the key ambiguous. + (list :query query :preamble-re preamble-re :action action + (if narrow + ;; Use bounds of narrowed portion of buffer. + (cons (point-min) (point-max)) + nil)))) (if-let* ((buffer-cache (gethash (current-buffer) org-ql-cache)) (query-cache (cadr buffer-cache)) (modified-tick (car buffer-cache)) (buffer-unmodified-p (eq (buffer-modified-tick) modified-tick)) - (cache-key (list query action narrow)) - (cached-result (gethash cache-key query-cache))) + (cached-result (gethash query-cache-key query-cache))) (pcase cached-result ('org-ql-nil nil) (_ cached-result)) @@ -558,10 +563,10 @@ replace the clause with a preamble." (puthash (current-buffer) (list (buffer-modified-tick) (let ((table (make-hash-table :test 'org-ql-hash-test))) - (puthash args (or new-result 'org-ql-nil) table) + (puthash query-cache-key (or new-result 'org-ql-nil) table) table)) org-ql-cache)) - (t (puthash args (or new-result 'org-ql-nil) query-cache))) + (t (puthash query-cache-key (or new-result 'org-ql-nil) query-cache))) new-result)))) (cl-defun org-ql--select (&key preamble-re predicate action narrow &allow-other-keys)