Conversation
c33c78e to
6c8ea8f
Compare
6c8ea8f to
a015bf2
Compare
592ffb3 to
d951470
Compare
19b0eaf to
93a306c
Compare
34b34d7 to
60a9679
Compare
60a9679 to
8e0fa89
Compare
ronag
left a comment
There was a problem hiding this comment.
- Unnecessarily complicated by locking.
- How does it "prune" i.e. free space?
- Why errorCallback?
|
IMHO I would think you could do this quite similar to the "simplified" memory store. |
bf40ed4 to
70fc6d3
Compare
Goes back to the discussion we were having in the original caching pr for how to handle errors, we decided to handle them silently so the request still goes through. Here it's just being used for handling json parse errors, since those shouldn't ever happen I would be in favor of just throwing outright though
It prunes the entire database of expired values when we come across one when looking up a key, we can make this more stricter though (i.e. run the purge query on every write), wdyt? |
70fc6d3 to
dd7ba9a
Compare
When the database is full (we are missing maxSize) we should remove entries whether they are expired or not. I think we should have something like, while beyond maxCount or maxSize drop 10% of the oldest entries. |
|
I omitted maxCount here since it would break if there were multiple processes using a db for cache If we do want it, I think we might be able to get away with it being stored somewhere in the db and being modified only with a procedure. |
I believe sqlite has meta commands for that, @mcollina ? |
lib/cache/sqlite-cache-store.js
Outdated
| * @returns {import('../../types/cache-interceptor.d.ts').default.GetResult | undefined} | ||
| */ | ||
| get (key) { | ||
| if (typeof key !== 'object') { |
There was a problem hiding this comment.
Of cache keys? Currently no
afa5d70 to
005f51b
Compare
That would still lead to the same state that I talked about here #3657 (comment)
|
005f51b to
f178cd1
Compare
|
CI is very red :( |
I think this would do it: PRAGMA page_count;
PRAGMA page_size;Then do |
@ronag I think this is possible, but I would likely investigate it after this lands. I think we would need to bring in https://www.npmjs.com/package/safe-stable-stringify to be able to have "stable" JSONs. |
What's wrong with e.g: function makeKey({ method, origin, path, vary }) {
const hasher = crypto.createHash('sha256')
hasher.update(origin.toLowerCase())
hasher.update(method.toLowerCase())
hasher.update(path ?? '')
hasher.update(vary?.map(x => x.toLowerCase()).sort().join(',') ?? '')
return hasher.digest('hex')
} |
|
That would be very slow unfortunately :(. Unless we really need it. |
Co-authored-by: Robert Nagy <[email protected]> Co-authored-by: Isak Törnros <[email protected]> Co-authored-by: Matteo Collina <[email protected]> Signed-off-by: flakey5 <[email protected]>
27f1d63 to
c40b97a
Compare
|
@mcollina I would still argue that using a hash id would be a better solution. I think the performance overhead is negligible: It can be done in 284.36 ns/iter which I would argue is probably similar to the overhead of doing a sql lookup of non-primary key (and also the extra lookup for our "upsert" in write). e.g. import { run, bench } from 'mitata'
import crypto from 'crypto'
import xxhash from 'xxhash-wasm'
const xxHasher = await xxhash()
const origin = 'https://example.com'
const method = 'GET'
const path = '/foo/bar'
const vary = ['Accept', 'Accept-Encoding']
function generateKey () {
let key = ''
key += origin.toLowerCase() + '\uFFFF'
key += method.toLowerCase() + '\uFFFF'
key += (path ?? '') + '\uFFFF'
if (vary?.length) {
for (const v of vary.map(x => x.toLowerCase()).sort()) {
key += v + ','
}
}
return key
}
bench('sha256', () => {
return crypto.createHash('sha256').update(generateKey()).digest().readIntBE(0, 4)
})
bench('xxhash', () => {
return xxHasher.h32(generateKey())
})
run()Though we would need to add xxhash-wasm as a dependency :/ |
|
@ronag I'm going to land this and open a fresh issue for the follow-up. I have an idea. |
Note: this is a draft since #3562 isn't landed. Until it is landed, this will be based off of that pr's branch. For the actual diff see flakey5/undici@flakey5/3231...flakey5:undici:flakey5/20240910/sqlite-cache-storeThis relates to...
Adding client side http caching (#3562)
Rationale
Adds a sqlite cache store option given the
--experimental-sqliteflag is providedChanges
Features
Bug Fixes
n/a
Breaking Changes and Deprecations
n/a
Status
cc @mcollina @ronag