Allow Filesystems to serve pre-parsed templates

[wmnnd]

Hey there, I was building a Liquid template that required a couple of render statements which would include template files from the local file system. I noticed that this had quite an impact on performance, and my suspicion is that it might be because Solid keeps re-reading and re-parsing each included template file from the filesystem (https://github.com/edgurgel/solid/blob/main/lib/solid/tag/render.ex#L54).

Would you be open to a PR that changes the signature of the FileSystem.read_template_file/2 callback from read_template_file(binary(), options :: any()) :: String.t() to read_template_file(binary(), options :: any()) :: String.t() | Template.t() ? I’d also adjust the render tag so that it handles this correctly.

[edgurgel]

@wmnnd I wonder if we should instead build a cache inside Solid to keep these templates that were already parsed and avoid reaching out to the FileSystem adapter?

And potentially ship with a simple implementation but allowing adapters to be swapped so that people can change how they want the cache to behave?

WDYT?

[wmnnd]

I was thinking that the FileSystem adapter might be the right place to add caching because if you have a custom adapter, you probably also want custom caching/cache invalidation logic. My idea would be to modify the LocalFileSystem adapter to accept an cache_templates parameter and then implement the caching logic directly in the adapter. Like this, everything would be explicit and backwards-compatible.

Adding the caching directly to the render tag would probably be less flexible and might also not be fully backwards-compatible. But there is probably not the one right way to do it :-)

[edgurgel]

Not sure how adding cache to render would be less flexible and not fully backwards-compatible :thinking: Could you elaborate on why?

The main problem with this approach, IMHO, is that the file system adapter is not anymore simulating a file system but a parser as well, given that the parsing step would need to happen at that stage passing the original Solid parsing options and this would also need to be used to cache as when the options change the parsing might also change (extra custom tags etc).

With a cache adapter the only concern is to store and retrieve already parsed templates and can be re-used against different file system adapters.

Imagine I have an adapter to use Redis as the cache layer and filesystem adapters for S3 and Google Cloud Storage. If there was no cache adapter then people need to implement Redis cache for S3 and Google Cloud Storage instead of re-using an existing one.

Also this approach is backwards-compatible as all file system adapters won't need to change.

[wmnnd]

Not sure how adding cache to render would be less flexible and not fully backwards-compatible thinking Could you elaborate on why?

My concern is that the file system adapter would have to potentially handle cache invalidation - and if there is a cache that doesn't go to the file system adapter, that wouldn't be possible.

I set up a little naive experiment over here: https://github.com/wmnnd/solid/tree/cache-poc It compares three approaches:

1. Rendering without a cache
2. Rendering with a file/string cache
3. Rendering with a template cache.

You can run the benchmark with mix test test/cache_benchmark/cache_benchmark.exs.

What I’m seeing pretty consistently is that the biggest difference by far is between 1 and 2. 3 still has a significant edge over 2, but maybe not enough to warrant implementing more complex caching logic with adapters.

Here is an example of the timing I get for rendering a template with four render statements 10,000 times:

No Cache: 5791.382ms
File Cache: 860.607ms
Template Cache: 561.846ms

So what I’m thinking now is this: The biggest performance improvement can be achieved with the smallest amount of complexity, i.e. by just adding file/string-level caching with ets to LocalFileSystem, more or less the way I did it in the POC for LocalFileSystemWithFileCache.

Would you be open for a PR that adds an :ets_cache option to LocalFileSystem? This could be done without touching any other parts of the Solid codebase.

[edgurgel]

Hey sorry I didn't have time this last week but I will check it out later today or tomorrow!

[esse]

added in PR #127