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

[4.x] Nocache performance improvements #8956

Merged
merged 13 commits into from
Nov 14, 2023
Merged

[4.x] Nocache performance improvements #8956

merged 13 commits into from
Nov 14, 2023

Conversation

jasonvarga
Copy link
Member

@jasonvarga jasonvarga commented Nov 8, 2023

Fixes #7039

This PR improves performance issues around the nocache tag.

First, there was no garbage collection around the temporary views. The directory would just continue to fill up with files. This is resolved by deleting the file immediately after rendering it.

Next, the major issue seemed to be when unserializing the session object when visiting the page. Every nocache tag used on the page will have a corresponding region object. In each object, it will contain all the necessary contextual data at that point in the template. That can potentially be a lot of data. Each field in the context is an object, with a reference to an entry, fieldtype, etc. They all end up getting serialized and stored in the cache. If you've got a lot of nocache tags on the page (e.g. in a big loop) this can very quickly balloon out of control.

This is being solved in two ways:

  1. Rather than one single session object being cached that contains all the regions, all the regions are cached separately.
  2. The nocache tag will now only store the fields that are used in the template.

For example:

{{ nocache }}
  {{ collection:articles :limit="the_limit" }}
    <a href="{{ url }}">{{ title }}</a>
  {{ /collection:articles }}
{{ /nocache }}

Only collection, articles, the_limit, url, and title will be stored. (Any identifiers that are ambiguous like collection and articles here will be tracked since we can't really tell if you're intending to use it as a variable or tag.)

This is a huge improvement over storing everything (like title, url, slug, permalink, every field in the blueprint, collection, api_url, etc...).

If you want, you can even be more specific:

{{ nocache select="the_limit|url|title" }}

Quick benchmark:

On a page with collection tag looping through 200 entries, and a nocache tag in each one:

{{ collection:test }}
  {{ nocache }} {{ title }} {{ /nocache }}
{{ /collection:test }}
  • Before PR: 196MB
  • After PR: 8MB

Todo:

  • Prevent temporary views piling up
  • Fix large cached array of cached urls After some benchmarking, this didn't seem to be a problem to begin with.
  • Fix large cached session object for individual urls

@jasonvarga jasonvarga marked this pull request as ready for review November 14, 2023 16:24
@jasonvarga jasonvarga merged commit 4ef6169 into 4.x Nov 14, 2023
@jasonvarga jasonvarga deleted the nocache-performance branch November 14, 2023 17:01
@stanbridge-wcorrea
Copy link

@jasonvarga a bug has appeared after this PR, when I use a nocache tag I'm losing context reference on antlers inside that nocache tag.

For example:

{{ nocache }}

    {{ custom_tag }}

{{ nocache }}

My custom tag is using $this->context and is getting some variables that have been working before.

When I use statamic/cms version 4.33.0 it works fine. When I update to 4.34.0 (when your PR got merged) it breaks.

Is that an expected behaviour?

Should we lose context when dealing with the nocache tag?

Let me know if I should open an issue about this. For now I'm using 4.33.0

Thank you!

@duncanmcclean
Copy link
Member

Can you try manually specifying the fields like this?

{{ nocache select="url|title|description" }}

@stanbridge-wcorrea
Copy link

@duncanmcclean That worked, thanks. Should I do that everywhere or it will change?

@duncanmcclean
Copy link
Member

You should only need to do it where you're using the variables inside a tag. If you're using the variables inside a template like normal, they'll be available without any issues.

@stanbridge-wcorrea
Copy link

@duncanmcclean thank you :D

@jasonvarga
Copy link
Member Author

I'm going to revert the automatic nature of this PR, and make it opt in instead.

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.

nocache: Memory usage gets bigger and bigger
3 participants