Peformance concerns about blade-icons HOT 29 CLOSED

@ragulka actually, I've changed my mind. I'll try looking into shipping a default <x-icon in the next 0.5 release.

from blade-icons.

I'll try to look into all of this in the near future. Just haven't gotten to it yet with the Blade UI Kit launch and all. There's already a PR open that looks promising.

from blade-icons.

Okay I've made the change again and pushed it to prod. So far seems good, I'm caching the icons as part of the deploy process and verified it's in the bootstrap/cache folder on the server and yeah not seen any issues yet. I'll monitor it overnight and check the slowlogs, but from memory of last time, it was pretty immediate that it happened

from blade-icons.

@RyanTheAllmighty that's great to hear! I'll be tagging a 1.0.0 version soon.

from blade-icons.

@owenvoke in addition. Current way of working will stay.

from blade-icons.

Same issue here using owenvoke/blade-fontawesome , the page load was being delayed a whole second due to icon processing.

I solved the issue moving the 23 icons that we use to a resources/svg directory and removing the dependency to the whole set of icons from owenvoke/blade-fontawesome.

At this time, I think that it is better if a note for performance tunning is added to the README.

from blade-icons.

Hi there. I'm sorry it took me this long to get to this. I'm happy to say that the next version of Blade Icons will include icon caching that will solve the existing performance problems. I'm currently looking for testers of the caching feature and would appreciate feedback on there PR here: #121 (comment)

from blade-icons.

@RyanTheAllmighty would very much appreciate it if you could help test this one out :)

Definitely. Will give it a shot in the next day or two and report back. Will be nice to get off the old depreciated package

from blade-icons.

Hey @RyanTheAllmighty, did you already had a chance to try it out? The reason I ask is that I want to make sure it fixes things for you before I tag a 1.0.0 release. I also saw a slight increase in CPU locally so want to make sure it's not too big of a deal for you on production. Let me know!

Yup. Been an insane week, and with Laracon today, didn't manage to find the time. I have some time tonight to take a look, I'll get back to you on my results in the next couple hours :)

from blade-icons.

We've had a lengthy debug session after we've deployed an update to Laravel + Filament v3 and this eventually turned out to be the culprit. As @lao9s described, we could not reproduce it locally but had massive performance issues on a server. We've debugged this using php-fpm slow log.

What's interesting though is that Filament provides their own workaround, which is not documented (https://github.com/filamentphp/filament/blob/e8d44231fcba1a0f4e9625afe7368447d56b2866/packages/panels/src/Http/Middleware/DisableBladeIconComponents.php).

protected $middleware = [
    ...
    \Filament\Http\Middleware\DisableBladeIconComponents::class
];

Simply include this middleware and the blade icons are not loaded anymore.

from blade-icons.

Yes, the problem with the new version is that we need the icon names up front to register the components. I'll think about a new way to do this, maybe with some sort of caching system. Perhaps through a command which can be run.

Not sure when I'll get to this but appreciating any help in the meantime. Thanks for reporting.

from blade-icons.

@RyanTheAllmighty one thing I'm wondering: does most of the performance hits come from reading the file or the registration of the components?

from blade-icons.

@driesvints looking at the slow logs, it's always the allFiles call that is taking more than 5 seconds.

I thought it might be that all the file reading on every request is tying up all the disk IO on the server, but looking at the server stats, that's not the case.

I did some basic debug logging in a testing environment, and the allFiles call is taking up a majority of the time and registering the components only taking about 10% of the time.

For instance with the largest fontawesome set of 1,000 icons taking around 20-25ms for allFiles and around 2ms for the registering of all the components.

It might just be a case of with smaller number of requests coming in, it's fine and can handle it, but once you have any larger number of requests come in, it starts to take an effect as it's constantly rereading all the files in and registering all the components.

from blade-icons.

@RyanTheAllmighty gotcha. Thanks for all your investigation here 👍

I think there's two things we can do here:

1. Cache the component names in a separate file similar to the packages.php from Laravel. That way we only need to read one file instead of using allFiles all the time
2. Provide a way for users to manually build that list so the library only knows about the listed icons and not every icon from a set.

I think 1. is a given that we should implement with 2. being a nice to have. I'll try to check into this later on but at the moment I'm a bit busy getting the Blade UI Kit release out of the door with a couple of other things after that. If anyone wants to help out here before I get to it I'll definitely appreciate it.

from blade-icons.

I was using blade-svg before and built my own blade icon component around it. What I did was to simply have the icon name provided as an attribute:

<x-icon name="chevron-down" />

Wouldn't a similar approach work?

from blade-icons.

@ragulka no because then you'll lose the benefit of a much nicer api to reference the icons.

from blade-icons.

I guess that's a matter of taste. I actually prefer typing out the attribute. It also has the benefit of being able to provide the icon name dynamically:

<x-icon :name="$iconName" />

It's quite useful when generating a bunch of HTML, like table headers, where you have an array for input:

$headers = [
    [
        'title' => 'Name',
        'icon' => 'user'
    ],
    [
        'title' => 'Company',
        'icon' => 'briefcase'
    ]
]

@foreach($headers as $header)
    <tr>
        <th><x-icon :name="$header['icon']" /> {{ $title }}</th>
    </tr>
@endforeach

As far as I understand, this is not possible with the current API (except for when using the blade directive)?

from blade-icons.

@ragulka you can still use the directive for that. It works exactly the same. I don't have any plans for implementing the above, sorry.

from blade-icons.

@driesvints, will that be in addition to the x-icon-${name} syntax? Or will <x-icon :name="$foo"> be the new syntax?

from blade-icons.

Hi Dries,

Really enjoying the package and the fact that we can pull in different icon sets easily!

I'm also experiencing performance issues after I pulled in Blade Material Design Icons add on.

Took me a little while yesterday to realise what it was, but running my test suite normally takes ~30secs but after pulling in the above package, it's now taking ~1min40secs.

For reference, there is 5,536 icons in that package.

Do you require any more information other than what @RyanTheAllmighty has already provided?
If so I'm happy to run some tests for you.

from blade-icons.

I would like to know also about using components for icons even as suggested with <x-icon name="icon-class">, doesn't mean this that for each icons there is an inclusion of component file? I am often concerned to not load too many views for example for layout like head, navigation, sidebar, footer, etc, and try to put everything in main layout app.blade.php for avoid too many views inclusion.

I was surprise to see a component for just an icon. Was not better a blade directive in this case? I see there is also directive, but I am asking why make also components.
As I understand for each component, even if are the same, there is a file inclusion.

Thanks

from blade-icons.

@thewebartisan7 I much prefer the component syntax over the directive. It's there so people can choose what they want to use. I'm still planning on introducing an <x-icon name=""> style component. Haven't gotten to that yet.

from blade-icons.

I see that you are working on single component with name prop. However, what I asked was even using single component if a page have 10 icons or more, this mean that for each icons there is a templates rendered of component?

I understand that you prefer more the component syntax, I also think is better. But I am concerned about too many views templates being rendered in a page.

For example I often use icon question in a form for each fields to show info in tooltip or inline, then menu have icons, etc.

from blade-icons.

#85 should do the job by caching them.

from blade-icons.

@RyanTheAllmighty would very much appreciate it if you could help test this one out :)

from blade-icons.

Hey @RyanTheAllmighty, did you already had a chance to try it out? The reason I ask is that I want to make sure it fixes things for you before I tag a 1.0.0 release. I also saw a slight increase in CPU locally so want to make sure it's not too big of a deal for you on production. Let me know!

from blade-icons.

Thanks @RyanTheAllmighty!

from blade-icons.

Looks all good after sitting overnight. I'm fairly confident the issue is fixed now. Thanks a bunch @driesvints should be good to tag a release

from blade-icons.

I tried php artisan icons:cache, and the app is loading a little faster and there is not much difference.

• With cache ~1.4s
• Without cache: ~2s
• Without this package: ~275ms

I noticed that locally is working fast, the problem appears when you deploy the app on a VPS server.

from blade-icons.