No caching used in SiteService.GetAllAsync

[adrian-crisan625]

While investigating some performance issues, we noticed that the call to SiteService.GetAllAsync does not cache the result at all. The rest of methods that get data from SiteService (ex. GetByIdAsync) use the cache for the results.
Is there any reason why SiteService.GetAllAsync does not use cache? Or is it an oversight?

public async Task<IEnumerable> GetAllAsync()
{
var models = await _repo.GetAll();

if (models.Count() > 0)
{
    foreach (var model in models)
    {
        if (model.Logo != null && model.Logo.Id.HasValue)
        {
            await _factory.InitFieldAsync(model.Logo);
        }
    }
}
return models;

}

[adrian-crisan625]

Similar issue in AliasService.GetAllAsync

[tidyui]

This is by design, otherwise everything in the entire database would be cached after a single call to GetAll(...)

[adrian-crisan625]

It is not about everything in database. just the Site data. Thus in SiteService.GetAllAsync just cache and retrieve from cache the all site list, using a key like "ALL_SITES".
When a site data is added or changed, at SiteService.SaveAsync, where the RemoveFromCache is called you could also remove from cache the cached data for the whole list of site, using the cache key "ALL_SITES".

I saw this pattern used in other services as well. And it hurts performance in the end.

[tidyui]

I think the interesting question here is, when and how are you using SiteService.GetAllAsync. The reason for it not being cached is that all of the more specific get methods, i.e GetByIdAsync, GetByInternalIdAsync, GetByHostnameAsync and GetDefaultAsync are all cached and these are the methods being used in all performance critical operations in Piranha. The GetAllAsync method is only internally when either building up the cache mappings, or when listing the available sites inside the manager UI.

What I meant with "everything in the entire database would be cached" is that we don't cache models after calls to GetAll... in any of the services because of not overloading the cache.

[adrian-crisan625]

We are using it so that we don't expose to outside the id of the site. The site we use have a custom field we call "Label". That is used by our client app. Thus our API receives such a "label" and that needs to be matched to a site. For that we get all the sites and check which one has that label set on it.

[tidyui]

In this case I would create a custom service that has its own cache of items of something like this:

public class SiteMapping
{
   public Guid Id { get; set; }
   public string Label { get; set; }
}

This way the service could then have methods for fetching a site based on the label by injecting the ISiteService into the custom service and calling GetByIdAsync. You could also listen to the hooks for whenever a site is saved or deleted and purge the cache entry in your custom service, see https://piranhacms.org/docs/master/application/hooks for reference.

[adrian-crisan625]

This is exactly what we did, more or less.
But I was expecting this to be in Piranha API.
I could give you the example for PageService.GetAllAsync which caches the results by internally using the GetByIdAsync.
You can say this would gets only pages of type T, but if I put T as PageBase, then I get all pages in a site, with caching.

For mee it seems the services PageService and SiteService treat data differently, and, in my opinion, it should not be so.
Data regarding sites is way smaller in memory footprint than pages, so memory concerns should not play a vital role for getting all the sites (same for aliases).

[tidyui]

Yes I can see now that there is a discrepancy between how the different services works, I would assume this is an oversight in the content based repositories. We will take a look at how this could be amended to align better.