LRU cache for curie lookup #61
Conversation
|
Looks like Thi's changes were included here by mistake. |
refactor: extract struct payload implement: unit test demonstrating lru bug fix: lru-freshen: the case of oldest lrun and newest lrun fix: handling an empty linked list would be more work, and useless, so skip it refactor: extract define lru-remove refactor: move all lru-evict calls to lru-ref amend: refactor: move all lru-evict calls to lru-ref refactor: use lru-remove from lru-evict refactor: remove unnecessary lru-evict call refactor: make all the counting work the same way renames/comments/formatting
| (time (apply make-db (cons path options))) | ||
| (apply make-db (cons path options))))) |
There was a problem hiding this comment.
Was there an issue using the flat form of apply?
medikanren/lru.rkt
Outdated
| (define (assert k st) | ||
| (if (not k) | ||
| (raise (format "assertion failure: ~a" st)) | ||
| #f)) |
There was a problem hiding this comment.
Dead code.
Otherwise, use when since the result won't be useful.
medikanren/lru.rkt
Outdated
| (if (>= (lru-num-entries ths) 2) ; freshen 1 entry is noop | ||
| (lru-freshen ths k) | ||
| #f) |
There was a problem hiding this comment.
It might make sense to put this short-circuit case into lru-freshen itself so its callers don't have to think about it.
|
|
||
| (struct payload (k v)) | ||
|
|
||
| (struct lrun ( |
There was a problem hiding this comment.
What does the n in lrun mean? n for node in a doubly-linked list?
|
|
||
|
|
||
| ;;; If the lru is full, remove the oldest entry. | ||
| (define (lru-evict ths) |
| #f)) | ||
|
|
||
| ;;; Make the entry with key k the newest entry. | ||
| (define (lru-freshen ths k) |
|
|
||
|
|
||
| ;;; Fetch item from lru cache, or if absent, from ref-behind. | ||
| (define (lru-ref ths k) |
| (begin | ||
| ; we are removing the oldest | ||
| (set-lru-lrun-oldest! ths lrun2) | ||
| (set-lrun-older! lrun2 #f))) |
There was a problem hiding this comment.
lrun2 may be #f here. Line 73 already takes care of this though, so it should be fine to delete this line.
| (begin | ||
| ; we are removing the newest | ||
| (set-lru-lrun-newest! ths lrun0) | ||
| (set-lrun-newer! lrun0 #f))) |
There was a problem hiding this comment.
Same issue with lrun0 possibly being #f here. See the other comment.
Benchmark exercise:
Database list:
Configurations:
The current pull request is to merge with LRU caching with no change to default behavior. That is, config.defaults.scm is configuration 1).
After merging, I propose another team member use configuration similar to 2) for a few days, and if successful, make it default.
Note that this PR includes #59 because #59 fixes the benchmark exercise above.