Cache the SVG icons serverside to boost performance

[kjac]

Prerequisites

• I have added steps to test this contribution in the description below

Description

This is a slightly more memory optimized approach to the same as #9198

---

This item has been added to our backlog AB#10549

[nathanwoulfe]

Benefit is probably negligible, but would we consider changing the cached object from IconModel[] to Dictionary<string, IconModel> so that we can access directly by index rather than using Linq and having to traverse the entire set (or rather, traverse potentially the entire set).

Building the cached object is the expensive operation, but if there's a perf gain via dictionary... Although, when calling GetAllIcons, we'd then need to convert the dictionary back to a list so might lose any benefit. Although although, there may be benefits in returning the dictionary - for the same reason, if we need a single icon from the full set, will be faster to get by index compared to filtering the 600+ items.

[kjac]

@nathanwoulfe good point. I'll have a look; single icons are fetched way more often than the full set, so definitively worth doing.

[kjac]

Right. So... it won't get any quicker using a dictionary. On the contrary, I'm afraid. Since we need to be case invariant when serving icons, we'll end up here real quick:

[kjac]

Hmmm unless we use a case insensitive dictionary. Food for thought 🤔

[kjac]

Ya... invariant dictionary works 👍

[nathanwoulfe]

I'll need to update this to incorporate the conventions discussed in #8884, to only check app_plugins/*/backoffice/icons, which will be more efficient. Also want to see if there's benefit in using Directory.GetFileSystemInfos() rather than EnumerateDirectories and EnumerateFiles (which in turn are replacing GetDirectories and GetFiles

ping @nul800sebastiaan @kjac @skttl @MMasey as you all have a vested interest in icons, svgs, icon pickers and the like 😄

[nathanwoulfe]

Trying to do this using the naming convention, and immediately hitting issues - OsmMaps package doesn't have a /backoffice/ directory, so not sure how to look for /backoffice/icons without first separately checking /backoffice/ exists, which seems super inefficient.

[skttl]

@nathanwoulfe would a simple Directory.Exists($"{pluginPath}"/backoffice/icons") do the trick for checking for the icon dir?

[nathanwoulfe]

@skttl probably would do the trick - can get all the plugin directory paths via enumerateDirectories on the app_plugins directory, then collect all the icon n directory paths, then iterate those to get the icon info.

Will see what I can do

[nathanwoulfe]

For those of us playing along at home... I've updated the PR with some new logic in the GetAllIconNames method, to do this:

• iterate the directories (top-level) of app_plugins
• in each directory, check that /backoffice/icons exists
• if it does, get all the svg files in that directory
• add those files to a collection
  This then returns to the previous logic:
• get all icons from the global icons path
• add the icons that don't already exist
• return the collated list of names

Probably should add a check too inside each app_plugin iteration, to ensure that any discovered icons haven't already been added.

I've done a really quick test, no benchmarking or anything, and it works fine. Need to confirm if performance is ok.

[skttl]

@nathanwoulfe icons in App_Plugins should overwrite default icons as stated in
#8884 (review)

Super cool @skttl! It should be the other way around though, if the icon doesn't exist in App_Plugins, then use the one that ships with Umbraco. That way you can override any icon that we ship with. As described here: #5606 (comment)

How to handle naming conflicts in different plugins is interesting though. I would probably opt for the simple way of just letting icons overwrite always, so if App_Plugins/A/backoffice/icons/icon-hello.svg gets overwritten by App_Plugins/B/backoffice/icons/icon-hello.svg

[nathanwoulfe]

@skttl yup, app_plugins icons are prioritised over the Umbraco-shipped ones. For conflicts in plugins, I'm taking the first instance of a name, then checking for collisions before adding subsequent icons. So it's the same logic for both cases - first instance found is the one that is kept.

[Shazwazza]

I can't remember if I mentioned or logged the other issue with these icons performance but the endpoint that loads them doesn't return the correct caching headers so the requests for icons are never cached at the browser level. At least this was the case before. This would instantly alleviate most perf issues with these icons and the hundreds(+) http requests for icons wouldn't actually be made since they'll be cached in the browser.

[nathanwoulfe]

@Shazwazza I'll have a look...

edit: the inverse is true - because IconController inherits UmbracoAuthorizedApiController which is decorated with the DisableBrowserCache attribute, we get no browser cache. There's a caching mechanism built into iconhelper.service, which caches each icon after the initial request.

Could improve that by requesting all icons on the initial load, and caching them the same way - since the initial request is cached on the server, it should perform pretty well, other than the very first request which primes the server cache.

[Shazwazza]

@nathanwoulfe I'm currently running some benchmarks in the back office for unrelated things but this GetIcon endpoint is seriously problematic 😭

because IconController inherits UmbracoAuthorizedApiController which is decorated with the DisableBrowserCache attribute, we get no browser cache. There's a caching mechanism built into iconhelper.service, which caches each icon after the initial request.

This is a big problem. Firstly, why does it inherit from UmbracoAuthorizedApiController? IMO this causes all sorts of issues that we've already had to work around like ensuring no icons are referenced on the login screen. They're icons, I don't see why they need to be protected. Second: the browser cache must be done correctly, the amount of requests is insane. Third: Probably the more important one, because this is an authorized endpoint it runs all sorts of security filters for the user which causes a titanic amount of DB queries to execute because this endpoint is called so many time and without browser cache.

If you put debug=true in your web config and then go to the back office with /umbraco?umbdebug=true to show the mini profiler - you will see what I mean. Navigate to a content item with a bunch of content and perhaps nested content and refresh, you will see mini profiler explode with a huge amount of DB queries, mostly from this.

To fix this:

• The endpoint should not inherit from UmbracoAuthorizedApiController. Any filters that it might need should just be applied directly to the endpoint.
• The endpoint should not require authorization
• It needs to have browser caching policies applied to it

[Shazwazza]

I've found another issue with regards to caching of a user's calculated start nodes. Caching isn't working correctly which is also why there are so many DB calls for these icon endpoints. I'll submit a PR shortly for that one.

[nathanwoulfe]

@Shazwazza I agree this doesn't need to be an authenticated endpoint. It's serving static assets.

Will try find some time to update

[Shazwazza]

Here's a related (and very important) PR #9778

[MMasey]

Hey @nathanwoulfe & @Shazwazza, I can shine some light in why the IconController inherits the UmbracoAuthorizedController. Although the svg icons are static files, there is also the possibility to add your own. At the moment this would have to be done in the same location as the main Umbraco SVG icons, but there is also work being done to import them via packages etc.

Because the icons could potentially come from a third party and it’s possible to run JavaScript from SVGs my initial idea around it was one of security. That being said, I also included the use of the HTML sanitiser which should strip out any script tags so as often with 20/20 hindsight it may have been a little overkill. I was unaware of the affect it would have on caching so it definitely make sense to get it fixed, thanks Shannon 🙂

[nathanwoulfe]

@MMasey I think sanitizing should be enough. Removing auth also solves the issues on the login screen (although those have been dealt with). Will have a poke around the PR and see what happens if I change the controller inheritance.

[nul800sebastiaan]

The failing test is unrelated.

[AndyButland]

Have reviewed code and run locally - looks good, and am seeing icons rendered as expected and can see the call to iterate the folders and find the icons is cached on each request for an icon.

Found one exception - if you have a site that doesn't have any plugins, the App_Plugins folder may not exist, so there should be a check for this. Something like this (in IconsService.GetAllIconNames():

var pluginIcons = new List<FileInfo>();

// add icons from plugins
var appPluginsDirectoryPath = IOHelper.MapPath(SystemDirectories.AppPlugins);
if (Directory.Exists(appPluginsDirectoryPath))
{
    var appPlugins = new DirectoryInfo(appPluginsDirectoryPath);
    ...

The rest aren't essential fixes, just suggestions to consider.

Looks like when it comes to plugin icons there's an issue with the file separator...

... which perhaps surprisingly actually doesn't seem to matter. But it might still be better to use MapPath to be explicit.

GetAllIconNames() - has an empty XML header which should be removed or completed. Perhaps can also rename this method to GetAllIconFiles() too? Just as it's not returning only the names,

Could consider using a HashSet instead of a List in this method, as that would help with the checking for duplicates.

Here's an implementation handling all these comments:

private IEnumerable<FileInfo> GetAllIconsFiles()
{
    var icons = new HashSet<FileInfo>(new CaseInsensitveFileInfoComparer());

    // add icons from plugins
    var appPluginsDirectoryPath = IOHelper.MapPath(SystemDirectories.AppPlugins);
    if (Directory.Exists(appPluginsDirectoryPath))
    {
        var appPlugins = new DirectoryInfo(appPluginsDirectoryPath);

        // iterate sub directories of app plugins
        foreach (var dir in appPlugins.EnumerateDirectories())
        {
            var iconPath = IOHelper.MapPath($"{SystemDirectories.AppPlugins}/{dir.Name}{SystemDirectories.AppPluginIcons}");
            if (Directory.Exists(iconPath))
            {
                var dirIcons = new DirectoryInfo(iconPath).EnumerateFiles("*.svg", SearchOption.TopDirectoryOnly);
                icons.UnionWith(dirIcons);
            }
        }
    }

    // add icons from IconsPath if not already added from plugins
    var coreIconsDirectory = new DirectoryInfo(IOHelper.MapPath($"{_globalSettings.IconsPath}/"));
    var coreIcons = coreIconsDirectory.GetFiles("*.svg");

    icons.UnionWith(coreIcons);

    return icons;
}

private class CaseInsensitveFileInfoComparer : IEqualityComparer<FileInfo>
{
    public bool Equals(FileInfo one, FileInfo two) => StringComparer.InvariantCultureIgnoreCase.Equals(one.Name, two.Name);

    public int GetHashCode(FileInfo item) => StringComparer.InvariantCultureIgnoreCase.GetHashCode(item.Name);
}

[nul800sebastiaan]

Good catch @AndyButland with the non-existing dir. I guess we never handled this before either. Alright, I've added your other corrections to this as well, looks good to me did another quick test to make sure App_Plugins still override core icons and that is the case. Will merge and cherry pick to 8.8+ for patch releases. 👍

Thanks for the feedback and efforts all! 🎉

[nathanwoulfe]

Great to see this merged (and improved, thanks @AndyButland). Go team!

[Shazwazza]

Hi all - just noticed a weird thing in the netcore branch will need to be tested - that's when Umbraco goes into upgrade mode and tries to redirect. Currently I get an error because it's trying to request: /umbraco/nullGetIcon?iconName=icon-search and errors out.

Might just be a quirk in this branch but want to make sure that we've tested the authorize upgrade and redirect functionality too, thanks!

[nathanwoulfe]

That's a strange one... There shouldn't be any need to request an individual icon, since they'l should all be available in the client-side dictionary. Can probably fix the error with a bit of refactor magic.

[nul800sebastiaan]

I bet it's because ClientDependency hasn't been updated yet so we're using the old js logic to call the endpoint that now no longer gets injected into the js collection at startup any more.

[Shazwazza]

This stuff isn't merged into netcore yet so still using the old way - but i just wanted to make sure with the new way that we've tested the upgrade process just to make sure.

[nul800sebastiaan]

Ah yes, I did run an upgrade to make sure, I think there was a console error, but nothing blocking the upgrade installer. I'll try again with debug=false to make sure.

[nul800sebastiaan]

I couldn't repro in v8/contrib, so I had an 8.10 site, copied the files over to upgrade it, the upgrade installer didn't fail on the icon endpoint and completed successfully.