Skip to content

Cache user avatars, covers, and team logos to disk - #35892

Closed
bdach wants to merge 1 commit into
ppy:masterfrom
bdach:avatar-disk-cache
Closed

Cache user avatars, covers, and team logos to disk#35892
bdach wants to merge 1 commit into
ppy:masterfrom
bdach:avatar-disk-cache

Conversation

@bdach

@bdach bdach commented Dec 5, 2025

Copy link
Copy Markdown
Collaborator

RFC.

This PR adds a disk caching layer for user avatars, user covers, and team logos.

Screenshot 2025-12-05 at 11 54 15

The goals of doing this are as follows:

  1. To decrease the amount of assets downloaded by the game on every run, which directly correlates to bandwidth costs.
  2. To possibly speed up loading of assets because loading from disk should be faster than fetching from online even with the next caveat, which is
  3. I intend to use the "avatar store" conjured here as a convenient attachment point to resolving another issue which is that avatars have horrible aliasing artifacts from excessive downscaling without mipmaps, and my angle to resolving that would be to have the "avatar store" accept a target size specification, and downscale the asset to that target size at texture loader level, which would resolve the aliasing caused by excessive downscaling without having to figure out how to purge mipmapped assets.
  4. As a minor side effect this removes a long-standing TODO wherein avatars would just hardcode a.ppy.sh URLs.

The major downside here is needing to resolve one of the two hardest problems in computer science which is cache invalidation. Web already gives some tools to tackle this; avatar filenames as given by web contain a unix timestamp of time of upload of the asset, while covers and team flag filenames as given by web are hash-like strings which I'm told are actually hashes derived from the asset. This covers online changes to the assets. That said, it does not cover:

  • Assets getting corrupted when stored to disk. (Integrity of the assets can't be easily checked without, you know, fetching the asset. While covers and team flags contain a hash-like string in the filename, that string is not actually an SHA256 of the asset, because it seems like web does some post-processing of the asset that ends up changing the SHA.)
  • Assets being potentially modifiable by the end users to do stupid stuff. (See preceding point.)
  • This PR does not currently offer any timed eviction scheme for the cache for assets that are long-unused. Could have one, but unsure on what to do with that.

Gonna leave it here and see what everyone says. Would like @ppy/team-web to have at least a read-through of the caching scheme and see if it looks correct from their perspective.

@bdach
bdach requested a review from a team December 5, 2025 11:05
@bdach bdach self-assigned this Dec 5, 2025
@bdach bdach added type/performance Deals with performance regressions or fixes without changing functionality. area:online functionality Deals with online fetching / sending but don't change much on a surface UI level. labels Dec 5, 2025
@github-project-automation github-project-automation Bot moved this to Ready for work in osu! team task tracker Dec 5, 2025
@bdach bdach moved this from Ready for work to Pending Review in osu! team task tracker Dec 5, 2025
@nanaya

nanaya commented Dec 8, 2025

Copy link
Copy Markdown

is there reason to not just have some generic url-based cache? set some size limit and use mru or whatever expiration method. Can also look at http header for expiration policy if feeling fancy but I think most (if not all) assets will have different url when updated

I don't think there's plan to change any of the url format at the moment but there's no promise on that (unless we want to make one)

and maybe hash the file when first downloaded and store it somewhere and check it on first use...

@peppy

peppy commented Dec 8, 2025

Copy link
Copy Markdown
Member

I started to write up thoughts on this but generally agree with the url/request based method being optimal.

  • Expiring disk cache can get messy
  • I think we're relying on folders-per-user to remove old user avatars as it stands (which leads to)
  • Storing thousands of files/folder on disk in a single directory can lead to issues

It would be pretty nice if we could use the existing file store for this purpose, along with a realm table tracking the usage and expiration (similar to the linking table that exists for beatmap usage). It was engineered around the above concerns in one way or another and on paper, seems like a good fit.

@bdach

bdach commented Dec 8, 2025

Copy link
Copy Markdown
Collaborator Author

is there reason to not just have some generic url-based cache? set some size limit and use mru or whatever expiration method. Can also look at http header for expiration policy if feeling fancy but I think most (if not all) assets will have different url when updated

I guess not except for the lack of definition of "whatever expiration method".

Expiring disk cache can get messy

This is too vague for me to address in any way. Messy how? Expiring caches in general is messy.

  • I think we're relying on folders-per-user to remove old user avatars as it stands (which leads to)
  • Storing thousands of files/folder on disk in a single directory can lead to issues

How is it a concern here but not with the hashed file store wherein the first-letter-of-hash folders will have thousands of files inside? Not trying to be antagonistic, just genuinely confused.

It would be pretty nice if we could use the existing file store for this purpose, along with a realm table tracking the usage and expiration (similar to the linking table that exists for beatmap usage). It was engineered around the above concerns in one way or another and on paper, seems like a good fit.

I have concerns that using realm will not only be slow of itself but make everything else slower because more data in realm = longer refreshes = everything else slows down even if it's not related to the cache.

@nanaya

nanaya commented Dec 8, 2025

Copy link
Copy Markdown

I guess not except for the lack of definition of "whatever expiration method".

well I did say mru being one of the possibilities...

@peppy

peppy commented Dec 8, 2025

Copy link
Copy Markdown
Member

This is too vague for me to address in any way. Messy how? Expiring caches in general is messy.

Basically would require a full sweep of the file system, since there's no sensible indexing method. Then we are also relying on file modification times (worst case) or file access times (best case, but also disabled or not available on some file systems), unless you are encoding expiry into the filenames. Basically limited places to store auxiliary information about the nature of expiry.

How is it a concern here but not with the hashed file store wherein the first-letter-of-hash folders will have thousands of files inside? Not trying to be antagonistic, just genuinely confused.

The whole point of the separation based on prefix of hash is to avoid this in the first place. It's a minimal best-practice that has been adopted by other apps (I stole the idea from somewhere else, I can figure out where if you care, but also hinted at in some user experiences).

exfat: https://en.wikipedia.org/wiki/ExFAT (microsoft recommendation of 65k files per directory, 1 million files in a folder increases cluster size aka per file overheads to 128 kb and reduces performance significantly)
linux: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/6/html/performance_tuning_guide/ch07s03s02s02s02

generally we'd hope to keep things in the 10-100k range per folder just to follow standards. The prefix method makes that much more likely to be the case (reduces by two magnitudes, meaning we could have a total of 10 million files tracked with a max of 100,000 per folder, probably enough).

I have concerns that using realm will not only be slow of itself but make everything else slower because more data in realm = longer refreshes = everything else slows down even if it's not related to the cache.

I'm fine with either a dedicated realm or just using sqlite for this (and future similar cases). I think we can probably all agree that not using realm where possible is in our best interest...

@bdach

bdach commented Dec 8, 2025

Copy link
Copy Markdown
Collaborator Author

Well I guess this is a dead end either way so closing. May try again in a few days.

@bdach bdach closed this Dec 8, 2025
@github-project-automation github-project-automation Bot moved this from Pending Review to Done in osu! team task tracker Dec 8, 2025
@peppy

peppy commented Dec 8, 2025

Copy link
Copy Markdown
Member

I intend to use the "avatar store" conjured here as a convenient attachment point to resolving another issue which is that avatars have horrible aliasing artifacts from excessive downscaling without mipmaps, and my angle to resolving that would be to have the "avatar store" accept a target size specification, and downscale the asset to that target size at texture loader level, which would resolve the aliasing caused by excessive downscaling without having to figure out how to purge mipmapped assets.

Also I was thinking the same thing, but I was also thinking about caching the downsampled versions to disk. Maybe that's one step too far and doing it dynamically is enough, given that we're working with sub-1024x1024 images in the first place.

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

Labels

area:online functionality Deals with online fetching / sending but don't change much on a surface UI level. size/L type/performance Deals with performance regressions or fixes without changing functionality.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants