(feat): Increase speed of org-roam-node-find

[vherrmann]

Increase speed of org-roam-node-find by caching last results. The validity of the cache is ensured by comparing the modification attribute of the database file with the last modification time of the cache.

This commit only increases the speed if you didn't modify any node since the last time you used org-roam-node-find.

[akashpal-21]

causes some regressions, particularly regarding using filter functions over the nodes,

[vherrmann]

Please elaborate what the problem is.

[akashpal-21]

I was using this advice function before https://github.com/Konubinix/Devel/blob/30d0184db0a61f46ca35e102302d707fef964a8c/elfiles/config/after-loads/KONIX_AL-org-roam.el#L770-L787

And I was also using filter functions such as described in https://org-roam.discourse.group/t/filter-org-roam-node-find-insert-using-tags-and-folders/1907/6

The problem in that was that caching org-roam-node-read--completions meant the filter functions just wouldn't work.

Then I tried this patch - and it had the same problem, caching meant that filter functions wouldn't work - although in the earlier case I was able to resolve it by using let to not use the cache within the scope of the functions, but I don't understand how to achieve a solution with this patch -

If it caches the last result - the advice function is much simpler and does the same thing? But I cannot answer with certainty because I wouldn't understand the modifications you made very thoroughly - just thought it would be prudent to let you know it causes this problem.

[vherrmann]

The cache from https://github.com/Konubinix/Devel/blob/30d0184db0a61f46ca35e102302d707fef964a8c/elfiles/config/after-loads/KONIX_AL-org-roam.el#L770-L787 works on a different level. It's caching the whole evaluation of org-roam-node-read--completions, while this pr doesn't cache the sorting of the entries. This pr also caches org-roam-node-list, so that even if you use a different filter-function at least the db doesn't have to be queried again.

Also this pr works at least for me with filter functions.

[akashpal-21]

I just checked again! Its working. I am so sorry I dont know what was going wrong previously. I feel so embarrassed. My filter functions are working just fine! Thanks, I honestly don't know why they werent working previously.

Thanks for this PR !

[vherrmann]

I added caching for sort-fn. This makes it viable to add sth like

(setq iko/org-roam-node-find-precache-timer
        (run-with-idle-timer 6 t (defun iko/org-roam-node-find-precache ()
                                   (org-roam-node-read--completions org-roam-node--candidate-list-prev-filter-fn
                                                                    (or org-roam-node--candidate-list-prev-sort-fn
                                                                        (when org-roam-node-default-sort
                                                                          (intern (concat "org-roam-node-read-sort-by-"
                                                                                          (symbol-name org-roam-node-default-sort)))))))))

to ones config. This code rebuilds the cache if needed after 6 seconds of inactivity.

[akashpal-21]

I added caching for sort-fn. This makes it viable to add sth like

(setq iko/org-roam-node-find-precache-timer
        (run-with-idle-timer 6 t (defun iko/org-roam-node-find-precache ()
                                   (org-roam-node-read--completions org-roam-node--candidate-list-prev-filter-fn
                                                                    (or org-roam-node--candidate-list-prev-sort-fn
                                                                        (when org-roam-node-default-sort
                                                                          (intern (concat "org-roam-node-read-sort-by-"
                                                                                          (symbol-name org-roam-node-default-sort)))))))))

to ones config. This code rebuilds the cache if needed after 6 seconds of inactivity.

Hi - I want to ask a query if you dont mind -- will the timer rebuild the sort of the existing cache or will it rebuild the cache every x seconds - even if no change in the db has occurred? I use vertico and it automatically handles sort for me based on history -- so if I dont need this sort rebuilt -- will the code work equivalently as before if I don't do any other input ?

Thanks.

[vherrmann]

It will only rebuild the cache if a change in the db has occured and emacs has been idle for 6 seconds.

[vherrmann]

Another trick i use is the following: I memoize org-roam-node-read--to-candidate:

(defvar org-roam-node-read--to-candidate--memoization-map (make-hash-table :test #'equal :size 1000))
;; FIXME: cleanup org-roam-node-read--to-candidate--memoization-map after long time
(defun org-roam-node-read--to-candidate (node template)
  "Return a minibuffer completion candidate given NODE.
TEMPLATE is the processed template used to format the entry."
  (let ((val (gethash (cons node template) org-roam-node-read--to-candidate--memoization-map)))
    (if val
        val
      (let* ((candidate-main (org-roam-node--format-entry
                              template
                              node
                              (1- (if (bufferp (current-buffer))
                                      (window-width) (frame-width)))))
             (res (cons (propertize candidate-main 'node node) node)))
        (puthash (cons node template) res org-roam-node-read--to-candidate--memoization-map)
        res))))

[akashpal-21]

Another trick i use is the following: I memoize org-roam-node-read--to-candidate:

(defvar org-roam-node-read--to-candidate--memoization-map (make-hash-table :test #'equal :size 1000))
;; FIXME: cleanup org-roam-node-read--to-candidate--memoization-map after long time
(defun org-roam-node-read--to-candidate (node template)
  "Return a minibuffer completion candidate given NODE.
TEMPLATE is the processed template used to format the entry."
  (let ((val (gethash (cons node template) org-roam-node-read--to-candidate--memoization-map)))
    (if val
        val
      (let* ((candidate-main (org-roam-node--format-entry
                              template
                              node
                              (1- (if (bufferp (current-buffer))
                                      (window-width) (frame-width)))))
             (res (cons (propertize candidate-main 'node node) node)))
        (puthash (cons node template) res org-roam-node-read--to-candidate--memoization-map)
        res))))

Can I use this hashtable in addition to this patch?
One thing still im still confused about -- because I was using the previous patch, and it worked perfectly except cases when I want to use the same filter-fn but with a different let scope setting its parameters -- I had to use a different filter-fn to reset the cache in between-- but its perfectly workable.

I dont really understand this sort fn thing -- can I update with the new patch and if I dont set the timer -- will it work equivalently as before?

Is the sort fn put so that the cache could be reset during idle time rather than on every save? Is it plausible to do delta update to the cache rather than a full reset on change ?

[vherrmann]

I dont really understand this sort fn thing -- can I update with the new patch and if I dont set the timer -- will it work equivalently as before?

Yes

Is the sort fn put so that the cache could be reset during idle time rather than on every save?

It's a needed optimization.

Is it plausible to do delta update to the cache rather than a full reset on change ?

No, this doesn't seem possible to me

[dmgerman]

The database query only takes a fraction of the total time. The actual culprit is org-roam-node-read--to-candidate.
Also, the formatting of the data coming out of the database takes also much more time than the actual query.

[vherrmann]

I profiled multiple rebuilds of the cache with the memoized version of org-roam-node-read--to-candidate.
I got the the following results: org-roam-node-read--to-candidate uses 30-60% of the samples, while org-roam-node-list uses 30-50% of them. Therefore the caching of org-roam-node-list seems to be a relevant improvement on performance, at least on my system.

[akashpal-21]

The current implementation can be improved -- I spent some time in concising the code. Currently I am trying a trio of cache, the cache of formatting improves responsiveness when the other two cache rebuilds after db change.

(defvar org-roam-node--cache-list nil)
(defvar org-roam-node--cache-time nil)
(defvar org-roam-node--cache-prev-filter-fn nil)
(defvar org-roam-node--cache-candidate-list nil)
(defvar cache/org-roam-node-read--to-candidate (make-hash-table :test 'equal))

(defun advice/org-roam-node-list (fn &optional force-refresh &rest args)
  (unless (and (not force-refresh)
	       org-roam-node--cache-list
	       org-roam-node--cache-time
	       (time-less-p
		(file-attribute-modification-time (file-attributes org-roam-db-location))
		org-roam-node--cache-time))
    (setq org-roam-node--cache-list (apply fn args))
    (setq org-roam-node--cache-time (current-time)))
  org-roam-node--cache-list)

(defun advice/org-roam-node-read--completions (fn &optional filter-fn sort-fn &rest args)
  (unless (and (eq filter-fn org-roam-node--cache-prev-filter-fn)
	       org-roam-node--cache-candidate-list
               org-roam-node--cache-time
	       (time-less-p
		(file-attribute-modification-time (file-attributes org-roam-db-location))
		org-roam-node--cache-time))
    (setq org-roam-node--cache-candidate-list (apply fn filter-fn sort-fn args))
    (setq org-roam-node--cache-time (current-time))
    (setq org-roam-node--cache-prev-filter-fn filter-fn))
  org-roam-node--cache-candidate-list)

(defun advice/org-roam-node-read--to-candidate (fn &rest args)
  (or (gethash args cache/org-roam-node-read--to-candidate)
      (let ((result (apply fn args)))
        (puthash args result cache/org-roam-node-read--to-candidate)
        result)))

(advice-add 'org-roam-node-list :around #'advice/org-roam-node-list)
(advice-add 'org-roam-node-read--completions :around #'advice/org-roam-node-read--completions)
(advice-add 'org-roam-node-read--to-candidate :around #'advice/org-roam-node-read--to-candidate)

Please remove redundancies -- actual code requirement is very small.

[dmgerman]

I think whether the user uses the advice or not should be optional.

All of these solutions are intrusive, some more than others. Thus, the user should be confronted with the choice of enabling the advice or not (specially the ones caching the database).

In many ways, these optimizations can live in a separate file (or even a module) and only be enabled (one at a time) if so desired. Also, org-roam uses dynamic scoping, so it is possible to create a file/module that contains redefinitions of functions in org-roam functions (and get fully tested) before they replace the originals.

by the way, I run the emacs profiler very little time is actually spent in the query. Almost all the time is spent building the data structures (including the reformatting of the query--the cl-loop in org-roam-node-list).

[akashpal-21]

I think whether the user uses the advice or not should be optional.

All of these solutions are intrusive, some more than others. Thus, the user should be confronted with the choice of enabling the advice or not (specially the ones caching the database).

In many ways, these optimizations can live in a separate file (or even a module) and only be enabled (one at a time) if so desired. Also, org-roam uses dynamic scoping, so it is possible to create a file/module that contains redefinitions of functions in org-roam functions (and get fully tested) before they replace the originals.

by the way, I run the emacs profiler very little time is actually spent in the query. Almost all the time is spent building the data structures (including the reformatting of the query--the cl-loop in org-roam-node-list).

Based on your suggestions - I prepared the final version -- three layers of cache is defined,
The only difference from this PR is that for the Full Cache case - I disable any custom SORT FN since it makes the final cache redundant - file-atime sort is borked and works equivalently as the file-mtime case -- and it works by the structure of the program. Custom SORT FN users should only use till cache level 2,

For most scenarios Basic Cache over the formatting would suffice, and it is the only one enabled by default
Ofcourse users should simply look to pluck out their use case and use the appropriate one -- but in the gist I provide documentation as to what is happening and what is being done more clearly to help them make an informed decision
in a the most accessible way.

Best.

https://gist.github.com/akashpal-21/5991e99949af5feb0f75729374ce3d66

[vherrmann]

I'm using sort functions. It is relevant for me that the cache works with sort functions. I don't think that code that silently uses the previous cache (so that the sort function is not applied) can be called a final version. Also due to the (when (>= org-roam-node-cache-level …)) blocks the code only works on a switch of the value of org-roam-node-cache-level, if you specify (setq org-roam-node-cache-level 3) before loading the file.

[akashpal-21]

I'm using sort functions. It is relevant for me that the cache works with sort functions. I don't think that code that silently uses the previous cache (so that the sort function is not applied) can be called a final version. Also due to the (when (>= org-roam-node-cache-level …)) blocks the code only works on a switch of the value of org-roam-node-cache-level, if you specify (setq org-roam-node-cache-level 3) before loading the file.

Yes, so as to not define things unless used. Why define a bunch of variables and functions if not used.

Coming to the sort fn -- if custom sort fn is used it invalidates the logic to use the cache over the final candidates - so its useless to cache and use a custom sort fn in the last case.

I do not want to define fns and variables unless used. Please provide solution if you may as how do dynamically define them like that.

[akashpal-21]

;; Processes ranked most expensive to least expensive
;; 1. `org-roam-node-read--to-candidate'                                  -- final formatting
;; 2. `org-roam-node-list'                                                -- initial formatting & db query 
;; 3. rest of `org-roam-node-read--completions' (filtering and sorting)

The last process is least expensive - it is not worth the bother to cache for both filter fns and then a new mechanism for sorting this list.

Sort fn invalidates fully the logic behind caching the last step from the point of being economic.

Cache level 1 already speeds up 98%
Level 2 99%
Level 3 the rest.

Should we increase complexity for saving 0.01 seconds?

[dmgerman]

Please try this function. It might obviate the need to cache the database. The cost of the processing of the list using append (in the original code) meant that the code was O(n^2). With mapcan (as in this version) the processing is O(n). If you have a a lot of nodes, this will be a huge improvement. Sqlite (even with 3 joins) is probably O(nlog(n)) on the number of nodes processed.

https://gist.github.com/dmgerman/53e266502e4d53294d1e8be46af93b4f

[akashpal-21]

Please try this function. It might obviate the need to cache the database. The cost of the processing of the list using append (in the original code) meant that the code was O(n^2). With mapcan (as in this version) the processing is O(n). If you have a a lot of nodes, this will be a huge improvement. Sqlite (even with 3 joins) is probably O(nlog(n)) on the number of nodes processed.

https://gist.github.com/dmgerman/53e266502e4d53294d1e8be46af93b4f

I saw your post the other day on the discourse - truly this is a good fix rather than trying to do a brute force cache over the nodes list for efficiency gains. But I couldn't replicate the case of org-roam-db-query deviating from the function by a noticeable margin -- in any case, increasing the efficiency of the function in generating the initial struct is a good step.

Anyhow - the cache over org-roam-node-read--candidates is a bad cache. Given the structure of the program if we cache the final format and also make the node-list function a little less expensive - the whole point of further caching the filter fn and make special provisions for user sort functions inside the cache falls through.

I will post my results on changing the function definition for a case of 20000 generated test sample later on your post in the discourse

[akashpal-21]

@dmgerman

## Control
(0.708954224 5 0.29964329199999895)
(0.732986124 5 0.31852763299999864)
(0.726951914 5 0.3131883000000002)
(0.740348504 5 0.31324887999999973)
(0.763081436 5 0.35603264399999546)
(0.749010954 5 0.33905975500000096)
(0.7884948519999999 5 0.38290975900000035)
(0.776870215 5 0.3572076250000009)
(0.766038528 5 0.35559597200000326)



# Experiment
(0.7393021 5 0.3376582859999999)
(0.72812542 5 0.317829279999998)
(0.719944593 5 0.31817142900000306)
(0.721954836 5 0.3187677640000004)
(0.719673294 5 0.3173475529999976)
(0.722364443 5 0.3200728679999969)
(0.7191277150000001 5 0.3170086490000017)
(0.722477972 5 0.3201192920000011)
(0.816113194 5 0.4135199800000038)
(0.7448088420000001 5 0.33939227799999827)
(0.738715821 5 0.3374332200000012)
(0.7387980009999999 5 0.3378341640000002)
(0.7979608579999999 5 0.3662030209999969)

Unfortunately I cannot spot any benchmark difference -- can you share yours? I have a simulated list of 20000 nodes - with no interconnections.

Do you see any noticeable difference in your benchmark runs ?

[dmgerman]

Mmm, see this:

https://org-roam.discourse.group/t/rewriting-org-roam-node-list-for-speed-it-is-not-sqlite/3475/3?u=dmg

I get a huge difference in garbage collection.

Can you run the following test (adapt to point to your org-roam database). This would show how fast the DB is:

https://gist.github.com/dmgerman/f8679f7a11bbdd748f648dd3f30151c5

This is how long it takes on my computer, with 1000 nodes:

dmg@cobalt:~/.emacs.d|master⚡ ⇒  /tmp/rip.sh
      66     173    1364
-- Loading resources from /Users/dmg/.sqliterc

real	0m0.009s
user	0m0.004s
sys	0m0.005s

it is basically instantaneous

[akashpal-21]

@vherrmann Fixed the case that changing the variable without reloading the file didn't work - a simple reloading of the mode would suffice - it was trivial enough to solve it. Also this allows the user to simply pluck out what they need and put in their init file -- the gist is simply for documentation - since this PR doesn't clarify anything, the user is left to his own ways to first patch it then manually comb through the changes and comparing with the original code to see anything being done or what even is being done.

Even custom sort fn users need not fear - the difference between cache level 2 and 3 is in milliseconds - no need to enable cache level 3. For node size < 3000 - the basic cache is more than enough.

https://gist.github.com/akashpal-21/5991e99949af5feb0f75729374ce3d66

@dmgerman I will continue our conversation in the discourse. But for the time being - these functions are a good bandaid over the problem.

[vherrmann]

This is intentional since any other sort other than the default `file-mtime' sort would make this cache for all purpose redundant.

Why would the cache of org-roam-node-read--to-candidate be redundant for a different sort? Regardless of the sort it will cache the result. Further the caching is meaningfull for more expensive sort functions.
Further I think that one should only optimize when it's truly needed. The optimization of not defining some of the variables and functions is at most marginal and makes the code harder to read. Further cached/org-roam-node-read--completions does not throw an error if the sort-fn is different to the last time. This can be highly confusing for people who just turn on org-roam-node-cache-level. An optimization should never change the semantics of a program.

[akashpal-21]

@vherrmann You're correct.

(defun cached/org-roam-node-read--completions (fn &optional filter-fn sort-fn &rest args)
  (let ((sort-fn (or sort-fn
                     (when org-roam-node-default-sort
                       (intern (concat "org-roam-node-read-sort-by-"
                                       (symbol-name org-roam-node-default-sort)))))))
    ;; Final Candidate cache nullification or changed FILTER-FN processing logic
    (unless (and (eq filter-fn org-roam-node-cache-prev-filter-fn)
		 org-roam-node-cache-time
		 org-roam-node-cache-candidate-list
		 (time-less-p
		  (file-attribute-modification-time (file-attributes org-roam-db-location))
		  org-roam-node-cache-time))
      (setq org-roam-node-cache-candidate-list (apply fn filter-fn sort-fn args))
      (setq org-roam-node-cache-time (current-time))
      (setq org-roam-node-cache-prev-filter-fn filter-fn)
      (setq org-roam-node-cache-prev-sort-fn sort-fn))
    ;; Changed SORT-FN processing logic
    (unless (eq sort-fn org-roam-node-cache-prev-sort-fn)
      (when sort-fn
	(setq org-roam-node-cache-candidate-list (funcall #'seq-sort sort-fn org-roam-node-cache-candidate-list))
	(setq org-roam-node-cache-prev-sort-fn sort-fn)))
    org-roam-node-cache-candidate-list))

My intention with not defining those functions and variables was not efficiency - I felt what is not being used should not be exposed to the user. Also it allowed me to slice the sections so that people may easily plug in what they need to their init -- otherwise I had to follow the established semantic of defining all variables first then all functions in order of their dependency and so on.. But it backfired, in that i found edebug to be non operational during test -- so I gave up, and refactored it to be more simple and in line with the orthodox way -- thanks for your suggestion.

I have defined the sort cache in the requisite way so as not to destroy the cache - the sort can be safely applied over the existing final candidate list - this way we make the cache better.

I have also defined the requisite variable.
Let me know if you have any other suggestions.

Thanks for your cooperation,
Best.