[Icons] Optimize Icons by inlining SVGs

[pierredup]

Q	A
Bug fix?	no
New feature?	yes
Issues	N/A
License	MIT

When using a lot of icons on a single page, each icon will, at best, trigger a filesystem lookup and at worst, do a network call to Iconify to load the icon. This can be bad for performance when a lot of icons needs to be loaded on the page. Setting up a decent cache pool can help with this performance, but can still end up in hundreds of request to the cache server for large pages.

This PR helps to optimize icons by inlining the SVG into the compiled template. This will only be done when using string literals for the icon name and all attribute keys and values. Any dynamic value used in the ux_icon function will fall back to the default behaviour.

[nicolas-grekas]

Is this reasoning still true when considering HTTP/2? To me this prevents efficient use of HTTP caching and uses the network to send low-priority bytes.

[pierredup]

The network calls are done server-side and does not happen in parallel, so HTTP/2 and HTTP caching doesn't play a role here.

A quick benchmark, loading a page with ~250 icons:

Case	Twig Render Time	Comments
Initial page load (No locally cached icons)	47747 ms	This is expected, since icons needs to be downloaded from Iconify first
Locally cached icons	24 ms
Inline SVG icons (this patch)	8 ms	~63% improvement

[pierredup]

One thing I also just realised, when replacing the local SVG file, the Twig cache would need to be cleared for the change to take affect, so this would either need to be opt-in, or a note added to the documentation

[nicolas-grekas]

Oh, I completely misunderstood the PR. Makes sense to me.
Just one issue: when renderIcon fails, we should fallback to regular runtime-resolution IMHO.

[smnandre]

Hey @pierredup ! Thank you for this nice PR

Hot reaction: I'm indeed not sure we want to enable such behaviour without some opt-in/out first :)

Couple of questions / comments:

I suppose it does not work with the twig components for now ?
Right now we bypass the component renderer to call directly the IconRenderer, with decent results

I started something back in february to "compile" the UX Icon Twig component back into twig function.
So this kind of optimization was on our mind, so there is something to digg here :)

Now i'm wondering.. do you have a IRL example of these 250 icons on a page ? Are they repeated ? Customized ?

PS : I'll open in an hour or two (update: tomorrow) a PR about icon configuration, i'm curious to see if this PR change anything :)

[javiereguiluz]

@smnandre about examples of pages with lots of icons, this page --> https://symfony.com/components has more than 500 icons:

But, it's a rare edge-case and all the other pages have less than 10.

[smnandre]

Thank you @javiereguiluz!

I think this is very good example of what the "reuse/symbol" we're working on (see #1800) can solve.
And it demonstrate the importance to add steps in a way that does not prevent future features or break existing ones.

But depending on how we implement these things, inlining could reveal itself a great advantage in rendering performance... but also the contrary if it prevents either runtime changes or symbol optimization.

So to be clear: i'm very, very interested in what you did there @pierredup, and i'd love to integrate twig compilation where we can or need !

(we worked on theses subjects during the early dev of this component and decided to postone it for the same reasons.. I don't find the step3 branch no more, but inlining was the idea.. the step2 was about converting the twig component into the ux_icon function right in the code.. something i'll need to implement asap because i'm moving stuff in the TwigComponent implementation and so the current Listener hack won't work anymore..)

Would you be ok to wait 10 days @pierredup and see then ?

With the configuration options, whatever we end un with for the prerenderers, and some experimental reuse/symbol system (or what we will have done then of course) ... we'll have all the cards on the table to chose what/when/how this compilation would offer the best advantage.

--

Regarding HTTP / cachewarmer / etc.. we now download the icons one by one. We could create some queue/buffer system to optimize it: multiple icons can be downloaded at once, as long they are in the same icon set...
For people using a favorite set this could represent a major gain in total download time...

[smnandre]

I've been thinking a lot about this PR and the nice work done here..

What if we use a flag/argument for a first version ?

Something like

{# Twig function #} 
{{ ux_icon('bi:circle', inlined: true) }}

{# Twig Component #} 
<twig:ux:icon name="github" inlined />

wdty @pierredup ?

[kbond]

In this case we'd throw an exception if it can't be inclined then?

[pierredup]

What if we use a flag/argument for a first version ?

@smnandre Would there be a scenario where a user doesn't want some icons to be inlined?

I think I would prefer a config option to globally enable/disable this for all icons

ux_icons:
    inline: true # default value is false

[smnandre]

I'm thinking of 4 things here:

• cache (file or config) that people will not know about
• n/bad access to network during twig cache build (this would make the compilation really long)
• conflict with other features
• impossibility for us to go back for BC break reasons

I'll work tonight on the sprite/reuse thing to see (maybe there would be no conflict)

That's why i'd suggest the following (without

2.20

• we add an optional manual flag in the config - experimental
• we do not set it in the recipe (to let options open for the future)

ux_icons:

    # Compile static icons on warm-up  (experimental)
    # foobar: true
    

2.22

• we set a default value (true) in the recipe
• we deprecated no setting this value --> at this moment we decide the default value

3.x

• defaut behaviour uses this decided value

What do you think ?

---

@pierredup i agree with you, a global config is best

I'm not so sure about the inline name, especially as we already inline in the HTML.
And let's keep in mind the enable/disable would not be "runtime"

What do you think:

• compile_on_build
• compile_in_template

Any other idea welcome here :)

[smnandre]

@pierredup I was going to add the configuration I asked for and merge this PR ...

.. but two things during my tests

1/ the HTML syntax is not precompil-able for now. Not a deal breaker to me. But this will be something we need to explicitely state in documentation

 {# will be compiled #}
{{ ux_icon('foo') }} 

{# will NOT be compiled #}
<twig:ux:icon name="foo" />

**2/ ** i have no idea how to simply configure this behaviour, as the node should not be services and I would like to avoid, as much as possible, to add this step in the hot path.

First though here:

• register a different "extension" depending on the configuration... the compiling one would be instanciated/decorated on warm up before it provides a specific FunctionNode in getFunctions()
• we could use a node visitor to do the same job but only during compilation
• ... any other ideas ?

wdyt ?

[kbond]

What about another function: ux_icon_inline()? If we can't use with html syntax, let's opt in this way.