nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.12k stars 4.03k forks source link

Optimize load times of images & icons #1066

Closed jancborchardt closed 5 years ago

jancborchardt commented 8 years ago

It would be good to reduce the number of HTTP requests necessary to load the icons. Currently every icon is its own file. Now that we don’t have PNG anymore it’s high time to optimize the SVG delivery.

We could and should optimize that by putting the SVG icons into a sprite. We can use http://iconizr.com for that. (Which uses https://github.com/jkphl/svg-sprite )

cc @nextcloud/designers and especially @juliushaertl @Henni because you seemed interested in that. :)

pixelipo commented 6 years ago

I did some more research regarding this problem. I still think that the #5985 is the direction we should head towards. It seems that Github is actually using a very similar solution: https://github.com/blog/2112-delivering-octicons-with-svg

Can anyone check if that's something that might work for us? From the text it is not clear at which point they are injecting SVG code into HTML...They were using icon fonts before, an approach that I was pondering as well, and didn't notice any performance change after making the switch.

Icon font is still an option for us, but there are some a11y problems related to it.

Is it possible to measure performance impact of #5985 to server?

@skjnldsv @nickvergessen @rullzer @MorrisJobke

EDIT: here's Github's injector helper script primer/octicons_helper

MorrisJobke commented 6 years ago

Can anyone check if that's something that might work for us? From the text it is not clear at which point they are injecting SVG code into HTML...

They do it in their ruby server. So it's basically the same as #5985

MorrisJobke commented 6 years ago

Is it possible to measure performance impact of #5985 to server?

The only problem I see there is, that the browser can not cache that asset and thus it is loaded all the time. On the other side: the server should have this in memory all the time anyways and just serves it. Only problem is, that we also do a lot of JS injection of icons, which is not possible with that approach. and would require to load the asset via browser anyway. Nevertheless it's worth a try.

benediktg commented 6 years ago

It would be good to reduce the number of HTTP requests necessary to load the icons.

With HTTP/2 this should not be that much of an issue anymore, I think. 😉

ChristophWurst commented 6 years ago

With HTTP/2 this should not be that much of an issue anymore, I think. wink

That, plus the move to Vue and more advanced file loaders like the url-loader that just inlines images smaller than a certain threshold make this less of an issue. I'll therefore close this ticket as there are now de facto standards that make the improvements possible without requiring us to implement these "hacks".

jancborchardt commented 6 years ago

No matter the outside technical developments, we still need to make sure to use all of the means we have to optimize performance. Not everyone checks out Nextcloud on a fast landline. :wink:

ChristophWurst commented 6 years ago

fast landline

:snail: 💨

MorrisJobke commented 6 years ago

Blowing snail? 🤯

bildschirmfoto 2018-10-03 um 00 17 01
ChristophWurst commented 6 years ago

well

bildschirmfoto von 2018-10-03 00-18-10

pixelipo commented 6 years ago

Looks like some work was done while I was on a "sabbatical" that adds the colour version programatically (e.g. svg/core/actions/more/fff?v=1). From a quick glance, that doesn't seem to be very high-performance - it uses up way more data than it should. Compare with the direct fetching of SVG in this table (transferred vs actual size): image

skjnldsv commented 6 years ago

@pixelipo, we add some strings to the svg on the colour api, so it's logical that it's bigger. Though the caching is working fine, so first load is long, second is instant :)

pixelipo commented 6 years ago

it's 10 times the size, which is a bit concerning...

skjnldsv commented 6 years ago

it's 10 times the size, which is a bit concerning...

Hum, it's more like 100Bytes more, or am I misreading your screenshot?

pixelipo commented 6 years ago

you're misreading it - in the first line, actual image size is 176bytes, but a total of 1.07 kilobytes were used up to fetch it. In case of a direct SVG fetch (line 5), that difference is practically negligible.

skjnldsv commented 6 years ago

Ah yes! How is that possible then? The data comes from the headers or something? Packet size?

rullzer commented 6 years ago

I don't buy that we add 900kb of headers.

@pixelipo any way to obtain more info on the request?

skjnldsv commented 6 years ago

I don't buy that we add 900kb of headers.

But it's 900 bytes, not KB? 1.07kb => 1070bytes, right?

rullzer commented 6 years ago

Right. I should not answer so fast. That might be. But all of it should be pretty aggressively cached as well

juliushaertl commented 6 years ago

I looked at some requests and the ~1kb of header data seems pretty reasonable, but there is not much we could do about this, besides merging icons to a sprite.

@pixelipo I also don't see any huge difference between the svg api endpoint and the direclty fetched svgs. Probably something related to the webserver setup?

pixelipo commented 6 years ago

There's a definitely a visible performance difference - the two icons in the screenshot bellow are very similar in size and the api version takes more than 10 times as long to load:

image

Here are few more examples of various filesizes: image

skjnldsv commented 6 years ago

@pixelipo I noticed the php script is slowing things down yes! I could use someone to check how to optimise things :) I'm guessing since we also get the svg and read the filesystem there is multiple factors here:

That's why we have hard caching :)

rullzer commented 6 years ago

Well yes there is overhead of course. But it should be 1 time.

I assume there is some theming going on and that is why we don't ship it as static asset right?

juliushaertl commented 5 years ago

Fixed with #12016 (using inline images in css instead of a sprite)