cms

cms copied to clipboard

Following on from my comments in #5254, here is a PR containing the basics.

Hopefully nothing too offensive in here. In my manual testing it hasn't introduced any regressions.

Ok, so it trashes the test suite. No problem, I will dig further into this.

I've been flipping between this code and the currently-released code and I'm more convinced now that the cachedFlattenedPages is the main culprit here. Just by sharing that cache for a single request, everything works so much faster.

Laravel 9 Statamic 3.3.8 PHP 8.0

Applying your PR to a pretty big site (6k entries total) nets me the following results for the homepage:

That means it's a ~6.3% performance improvement for my specific use case.

Granted, my local machine is somewhat slow, the same homepage on the staging server loads in ~1.3s.

Edit: outdated, see https://github.com/statamic/cms/pull/5899#issuecomment-1113930416, I had cache tags wrapped around my navs.

Thanks for trying it out and sharing that data @jelleroorda

Can I check, is that the Time To First Byte value that you've captured in your table? Or something else.

Yes, it's the TTFB or what they also call waiting time in Firefox.

Also, when you say "on the staging server loads in ~1.3s", is that with or without this patch?

It does depend on how many {{ nav }} tags are used on the page - so if you only have 1, it might not be as dramatic a change as I've seen on my environment, where we have a minimum of 3 on each page and on some pages up to 5 or 6.

And to note that this doesn't yet do the next step of introducing the layer of caching between requests. But that's probably a bigger PR.

The 1.3s is on the staging server without the patch.

The homepage does 7 nav renders, of which there are a few "duplicated" calls. A nav may get called three times, in the header (desktop + mobile variants), and the footer for example.

How can I use this PR on my statamic installation before it is merged into the main package? We are having lots of performance issues with the nav tag, even after using select and max_depth. we have about 4-6 nav tags per page. Also happy to provide any performance improvement statistics if you would give some instruction on how to record.

How can I use this PR on my statamic installation before it is merged into the main package? We are having lots of performance issues with the nav tag, even after using select and max_depth. we have about 4-6 nav tags per page. Also happy to provide any performance improvement statistics if you would give some instruction on how to record.

You can look into applying it as a Composer patch

TIL! Thanks @duncanmcclean

@duncanmcclean Thanks!

The 1.3s is on the staging server without the patch.

That's pretty speedy already @jelleroorda, are you doing any kind of caching around any of those navs? Or are you running things from a database?

Whoops, you're absolutely right @simonhamp! It's been a while since I worked on that site... I see that I've added some caching with cache tag {{ cache for="15 minutes" key="cache_tag_header_navigation" }} (with some matching invalidation).

Just now I eyeballed it again; with the cache tags disabled it runs in around 2.05s on the staging server. With the optimization it runs in around 1.6s which is a whopping ~22% speed improvement. Not sure how I still measures a ~6% speed improvement in my local environment though, since that also has the cache tags enabled.

Awesome! So you'll still see a speed up if some of those navs aren't cached.

My next PR (Coming Soon™️) takes this even further with caching of its own baked into the navs.

Not sure how I still measures a ~6% speed improvement in my local environment though, since that also has the cache tags enabled.

Docker?

Docker?

I’m using Mac with valet.

Different Stache/Cache drivers?

So, I've ended up adding the caching here. It was simpler than I thought... because the Tree Blink cache key is a hash of the actual tree, the cache is effectively automatically broken when it changes.

All that's needed is for that cache to expire, so I gave it a reasonable (and configurable) lifespan.

Different Stache/Cache drivers?

Well that’s not what I meant really. What I meant was that even though I was using the same set-up locally (with cache tags) and I still had a speed improvement. So it’s not about the local vs staging set-up.

I've tested this with the site I was working on when I originally posted this issue and the results are very promising.

This site does not output the exact same nav multiple times, each is only output once. It has three main navs, and a fourth structure is output on sub-pages to display that section only (taken directly from the pages collection, not a nav). The primary nav is fairly large (~50 items), the other two are small (~5 items). I ran tests on a sub-page that also displays the section pages (~10 items).

With fully augmented nav tags: {{ nav:primary }}

• Statamic 3.3.10: 3,136.804 [ms] (mean)
• simonhamp:patch-4: 2,146.576 [ms] (mean)

With partially augmented nav tags: {{ nav:primary select="title|url" }}

• Statamic 3.3.10: 1,016.283 [ms] (mean)
• simonhamp:patch-4: 549.216 [ms] (mean)

For me with this site the main cause of the slowdown is the full augmentation of the pages, which is what I was trying to solve with the custom nav tag I wrote. However that issue was fixed in 3.3 with the ability to select only the fields you want.

But even when only selecting the necessary fields, I'm still seeing a significant performance improvement of around 40% with the changes in this PR.

I tested with multiple runs of the ab tool: ab -n 12 -c 4 http://example.test/...

@simonhamp haven't had a chance to do any proper testing but there is a visually noticeable increase in speed since adding this patch. great job mate

@jackmcdade @jasonvarga @jesseleite i know this isn't necessarily top of the pile for you guys right now, but I'd be happy to help get this reviewed and merged into core.

We're facing up to some hefty penalties performance-wise right now which this branch solves, but we'd really like to not be running a fork in production.

Is there anything I can do to help expedite this?

You don't need to be running a fork, you should be able to use the composer patch solution mentioned earlier.

I've been out of the country dealing with some family issues. I'll do my best but I should be back at work next week.

@jasonvarga sorry, I didn't mean to disturb while you're out of office 😞 hope you can get done what you need. Thank you for taking some time to reply 🙏🏼

By 'fork' I was conceptually including the patching method too... while it's workable, it isn't sustainable in case of other updates. But we'll do so for now!

Left a pretty hefty final (? 🤞🏼) update on https://github.com/statamic/cms/issues/5254#issuecomment-1137661713 - I think this has really hit a home run now (also pleased with how many metaphors I've used throughout this whole thing)

The only thing I think this could benefit from (later, probably as another PR) is busting and building this cache when the Navs (or any of their attached Entries) get updated in the CP, as that would mean they're never stale and built and ready to go right away without requiring a visitor to hit a page to prime the caches.

I'm thinking when you're in dev you probably don't want this caching happening - it kind of gets in the way.

So I may change the behaviour of that a little so it's a bit more configurable/predictable.

I tried to do some further work here so that this cache was a bit more useful rather than just expiring after a certain amount of time.

But it's introduced too many regressions that I don't fully understand yet, so I forced this back to a state where the tests passed. I've opened a separate PR for that work to see if anyone can lend a hand with investigating what the heck is going on there.

The upshot of it is that in local dev, you may want to set this structures.cache_ttl to zero (0) so that you're not bumping into things not changing when you want them to.

Perhaps we should set it to default to zero?

@jasonvarga Is it possible to merge this PR as soon as possible? I've tested this with the nav tags on one of our pages and we could really use the improvement in terms of overall page loading time.

@simonhamp @jasonvarga are there any plans to finalize this PR and merge it anytime soon? It would help us a lot on some active projects.