cms

cms copied to clipboard

The raw class variable is only set once on initialisation, so it makes sense to only evaluate the augmented value once too.

This PR adds a new class variable called value, which caches the value after being called once.

Before:

After:

Nice!

Something I can see might happen is if an evaluated value ends up returning null or false, it will still try to evaluate it again each time.

Maybe you'll also need some sort of hasEvaluated property instead of just checking value.

Ahhh true! That one should be sorted now

As an extension to this, what are your thoughts on a cross request value cache? I understand the there's probably issues with the screenshot'd POC (invalidation logic, loads of cache save calls, and fieldtype duped handles being a few). But this has brought time spent on the value function down to 36ms

I'm not sure whether it's possible to cache further up, or at what point you'd rather people just use static caching 😅 Just thought it was worth noting what I'd found

I think it's a cool idea, but let's leave it for a separate PR. I think it will need extra brainpower. I'm thinking potential for race conditions, the cache value getting enormous, and more.

I remember we used to cache all the generated glide URLs to a single array. Eventually, as you generated more and more images, the array got so big that it started requiring more memory to unserialize it from the cache, and the site just crashed.

If we were to store all the augmented values in one big array, I could see the same problem. Potentially even sooner than the glide issue, since in there we were just storing URLs but in here we could be storing objects, etc.

By the way, are you able to provide a GitHub repo for a site where this PR has a noticeable effect? If it's private, you can shoot it over to [email protected]. That would be great.

Interesting, I'll have a think :) Sent an email through now

This is cool. I'm seeing around a ~100ms speed up on most page with this 👍🏼

This is really good stuff. Status?

Just been digging into a performance issue with a site and run into something that's kind of related to this, but not solved by this PR.

On this site (that uses Blade) I'm doing a lot of this:

{{ $entry->image ?? 'fallback.png' }}

What I've discovered is that is twice as slow as this:

{{ $entry->image }}

// Only look this up once so fetch doesn't affect timings
$original = Entry::find('my-id');
Benchmark::dd([
    'get' => function () use ($original) {
        $entry = clone $original;
        $test = $entry->image;
    },
    'get-null-coalescing' => function () use ($original) {
        $entry = clone $original;
        $test = $entry->image ?? 'fallback.png';
    },
], 10);

[
  "get" => "0.792ms"
  "get-null-coalescing" => "1.553ms"
]

I assume the reason is that the null coalescing causes the value to be evaluated twice, once to check for null and once to actually get the value.

However, this PR does not actually help in this situation because the evaluated value is stored in the Value object, but when you access an entry's value this way a brand new Value object is created every time.

The suggested changes are great, but have no effect on relations (terms or entries). Those have to get re-resolved for every time the parser finds them in the antlers template. The built and cached Value object only has the information that it is a Term relation of a specific type but holds no augmented values. The following code e. g. means that the same query for the relation to the industries taxonomy gets called twice:

{{ if industries }}
  {{ industries }} 
    {{ title }} 
  {{ /industries }}
{{ /if }}

This means in our case, where we have many lists with collection entries having up to 6 taxonomies, that many queries are made unnecessarily causing massive performance issues.

Closing this PR as it should be covered by the upcoming performance improvements. Thanks for understanding!