Skip to content

Simple API Caching for /libraries* requests#2343

Merged
advplyr merged 16 commits into
advplyr:masterfrom
mikiher:caching
Nov 26, 2023
Merged

Simple API Caching for /libraries* requests#2343
advplyr merged 16 commits into
advplyr:masterfrom
mikiher:caching

Conversation

@mikiher

@mikiher mikiher commented Nov 23, 2023

Copy link
Copy Markdown
Contributor

/libraries* API requests usually fire complex database queries, and those queries exhibit (at least on my setup) inconsistent latencies. In some cases, the same query takes 1-2 orders of magnitude longer to complete (I did not dig deeply into the reasons).
In addition, JSON-stringifying the query results is also expensive.

This adds a simple middleware for caching GET API /libraries* requests.

  • The cache is a simple in-memory LRU cache with a maximum size and maximum number of items.
  • Cache keys are user+url, and cache values are the raw response strings (post-stringify)
  • There's no TTL (except for one special case)
    • The only case in which a TTL is set is for /personalized requests, since these have a Discover component which returns random books from the library that change on every request. I thought a 30 minutes TTL is a good compromise for this feature. In the future, I think the various /personalized components should probably be requested and loaded separately, and in parallel
  • Cache is currently invalidated when any database change occurs (not optimal, but wanted to keep it very simple for V1).
    • Incidentally, The cache is also invalidated when the browser page is reloaded with F5/Ctrl-R (this is a side effect of a database user update that happens during socket re-authentication), which seems like a good thing.

This seems to improve UI response times visibly (especially when client and server are on the same host), and reduces UI hanging due to query latency inconsistencies.

@mikiher mikiher marked this pull request as ready for review November 23, 2023 08:52
Comment thread server/utils/timing.js Outdated
@advplyr

advplyr commented Nov 24, 2023

Copy link
Copy Markdown
Owner
  • Incidentally, The cache is also invalidated when the browser page is reloaded with F5/Ctrl-R (this is a side effect of a database user update that happens during socket re-authentication), which seems like a good thing.

This didn't seem necessary to me since the update happening on page refresh is the lastSeen of the User which doesn't impact any of the queries right now. I updated it so that the hook doesn't fire for that.

This is a great start for caching the API. Even on my local I can detect the improvement

@mikiher

mikiher commented Nov 25, 2023

Copy link
Copy Markdown
Contributor Author

Sorry about the file naming mistake - slipped from my attention.
BTW, were you planning to take a look at #2305 ?

@mikiher

mikiher commented Nov 25, 2023

Copy link
Copy Markdown
Contributor Author

Hi, please don't merge this yet.
There seems to be some problem with this PR when testing with the latest Android client.

@advplyr

advplyr commented Nov 25, 2023

Copy link
Copy Markdown
Owner

Yeah I can see that the cached results aren't setting the Content-Type header. Axios on the website must be parsing it as JSON anyway whereas the CapacitorHttp plugin is not.

The mobile app is receiving a string instead of a JSON object

@mikiher

mikiher commented Nov 25, 2023

Copy link
Copy Markdown
Contributor Author

OK, fixed now (and also fixed the test). Please take a look again. It now works nicely both on Chrome and and Android.

@advplyr

advplyr commented Nov 26, 2023

Copy link
Copy Markdown
Owner

Working beautifully, thanks!

@advplyr advplyr merged commit 5a8c60a into advplyr:master Nov 26, 2023
@tonyedwardspz

Copy link
Copy Markdown

Can't wait to see this in the next release. Thanks for the effort @mikiher 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants