Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "object cache" misfeature; add simpler "resource cache" #708

Merged
merged 15 commits into from
Apr 5, 2024

Conversation

danielballan
Copy link
Member

@danielballan danielballan commented Apr 3, 2024

The "object cache" was a misfeature because it mixed two distinct resource pools:

(1) Caching chunks of data
(2) Caching file handles

(I admit this should have been immediately clear at the time it was added. This was very early on when we were prototyping a lot of aspects to validate the general idea.)

This PR removes the "object cache" entirely. The only remaining mention is in the service configuration, where object_cache is still accepted, for backward-compatibility, but a warning is emitted that any configuration here has no effect:

$ git grep object_cache
tiled/config.py:        if "object_cache" in config:
tiled/config_schemas/service_configuration.yml:  object_cache:

Then the PR adds in its place a "resource cache" which:

  • addresses only file handles (2), not cached data (1)
  • involves much less custom code, using cachetools.TLRUCache to do the heavy lifting

Here we can see it in action. The file is only opened while processing the first request. The second request uses a cached file handle.

$ strace -fe open,openat tiled serve directory files/ --public

This is the relevant output. (Some noise related to #663 has been removed for clarity.)

[pid 2870299] openat(AT_FDCWD, "/home/dallan/Repos/bnl/tiled/files/a.tif", O_RDONLY|O_CLOEXEC) = 17
INFO:     127.0.0.1:57168 - "GET /api/v1/array/full/a HTTP/1.1" 200 OK
INFO:     127.0.0.1:57176 - "GET /api/v1/array/full/a HTTP/1.1" 200 OK
[pid 2871270] openat(AT_FDCWD, "/home/dallan/Repos/bnl/tiled/files/stuff.h5", O_RDONLY) = 18
INFO:     127.0.0.1:52242 - "GET /api/v1/array/full/stuff/data HTTP/1.1" 200 OK
INFO:     127.0.0.1:52246 - "GET /api/v1/array/full/stuff/data HTTP/1.1" 200 OK

After waiting for the cache expiry time (default 60 seconds) accessing it again opens the file again, as expected.

The cache parameters are tunable via environment variables, but this is intentionally not exposed through the service configuration or CLI. I think it's possible that we can choose sensible defaults that will not need to be overridden except in debugging scenarios.

Closes #683

@danielballan danielballan changed the title Remove "object cache" misfeature Remove "object cache" misfeature; add simpler "resource cache" Apr 3, 2024
@danielballan
Copy link
Member Author

A server side data cache, use case (1), is still of interest, but should be explored in future issues and PRs. The primary goal of this PR is to remove the object cache in preparation for a beta release, and to put in place a way to do basic file handle caching, which we know can be important, to avoid thrashing file handles when serving a burst of parallel requests.

@danielballan
Copy link
Member Author

One more interactive test. If the cache is zeroed out, the file is reopened on each request, as expected:

TILED_RESOURCE_CACHE_MAX_SIZE=0 strace -fe open,openat tiled serve directory files/ --public
...
[pid 2876503] openat(AT_FDCWD, "/home/dallan/Repos/bnl/tiled/files/stuff.h5", O_RDONLY) = 14
INFO:     127.0.0.1:43080 - "GET /api/v1/array/full/stuff/data HTTP/1.1" 200 OK
[pid 2876503] openat(AT_FDCWD, "/home/dallan/Repos/bnl/tiled/files/stuff.h5", O_RDONLY) = 12
INFO:     127.0.0.1:43088 - "GET /api/v1/array/full/stuff/data HTTP/1.1" 200 OK

Copy link
Contributor

@genematx genematx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the mechanics of with_resource_cache, and how it simplifies the code overall. Just a few notes from me; nothing major.

docs/source/explanations/caching.md Outdated Show resolved Hide resolved
tiled/adapters/resource_cache.py Outdated Show resolved Hide resolved
tiled/adapters/resource_cache.py Outdated Show resolved Hide resolved
tiled/adapters/resource_cache.py Show resolved Hide resolved
tiled/adapters/hdf5.py Outdated Show resolved Hide resolved
@danielballan danielballan merged commit 58bc300 into bluesky:main Apr 5, 2024
8 checks passed
@danielballan danielballan deleted the object-cachectomy branch April 5, 2024 14:04
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.

Remove "object cache" and replace it with something much simpler
4 participants