injectStyle css bloating the browser at tag declaration #1439

Closed
BirdieMcFly opened this Issue Dec 15, 2015 · 13 comments

Projects

None yet

4 participants

@BirdieMcFly

Hello,

we have been using RiotJS for building an application that has now dozens of tags (120 for now, each having more or less script and scoped style) and everything is going pretty well. However with the number of tags growing, we realized that the page loading increased exponentially with each new tag. We managed to find where the problem comes from.

During each tag declaration (when riot.tag2() function is called), riot injects styles in a style tag in the HTML head. That css injection is using the same tag again and again, by appending new css. This mechanism forces a DOM update and CSS parsing at every tag declaration. So when the number of tags grows, each new tag declaration will take more time, leading to an exponential loading time as the application grows.

I have made some examples on plunkr to illustrate the problem (we are using riot 2.2.4 in our project but the following examples are using latest riot 2.3 which has the same problem) : http://plnkr.co/edit/Fi3NlKv2RliMEkw8bl1w

Interesting datas are printed in the browser console. We have a bunch of tags, all with the exact same structure. As you can see in the console, each new tag declaration takes more time than the previous one. Total time exceeding 10000 ms on my computer, with the latest Chrome.

Next example is how we addressed that issue in our project, by hooking the style injection : http://plnkr.co/edit/Ill0vvfeGWOKICAN4th8

The tags file is the same than the previous example. In the console you can see that tag declaration time stays low all the way. Total time is less than 100ms on my computer.
The hook we use is basically storing css code in a variable, and every 10 calls of that css storing, a new style tag is created (and won't be updated after that).

I'm not sure this is the most elegant way to address that issue, however it points out that there could be a real performance improvement here, allowing large apps to live with good perfs. By putting that hook in our project, the loading time of the app went from ~ 4s to ~ 500 ms on my computer (and in those 500ms there is an ajax API call...).

Still Riot is awesome, keep up the good work 👍

@cognitom
Member

@BirdieMcFly great insight and research. Thanks. How about such options?:

  • Option A: load style at the first mount
  • Option B: provide an option to export CSS independently on pre-compilation

At this point, already we can use B with the custom style compiler, FYI.

@GianlucaGuarini
Member

This option is already enabled:

sudo npm install riot-cli -g # the new cli will be distributed with the next riot release
riot my/tags/folder tags.css --export css 
riot my/tags/folder tags.js --exclude css
@cognitom
Member

@GianlucaGuarini oh, really? Great!

@GianlucaGuarini
Member

@cognitom riot -h to know the new available options.. I will publish the new riot release with the new cli today or tomorrow, but you can test it already using npm install riot-cli -g

@cognitom
Member

Cool 😄

@BirdieMcFly

@cognitom for Option A: it would be very nice to have style injected on mount and removed when all instances of a given tag are unmounted. However i'm wondering if there is a way to do that and keep having good performances.
For Option B : I didn't want to feel obligated to pre-compile tags to address that issue. Compiling in browser has magically good performances and is more practical in a development phase. For now the only reason i would pre-compile for a production phase is to reduce the number of HTTP requests.
Also pre-compiling requires having a nodejs environment. This is not a heavy requirement but it doesn't tend to keep things as simple as they can be.

@nippur72
Contributor

As I see it, riot.tag() should not call injectStyle() directly but should accumulate css in a variable. Real style injection should occur in a single batch during riot.mount(). I can cook a PR if the idea is accepted.

Of course this can be already solved with pre-compiled tags, which is something you might want to do if you have 120 custom tags :-)

@GianlucaGuarini
Member

@nippur72 yes pr are welcome :)

@GianlucaGuarini GianlucaGuarini added the bug label Dec 15, 2015
@nippur72
Contributor

Here is @BirdieMcFly 's test case with the PR I'm working on.

To test it, just comment/uncomment the <!-- script>. On my PC it goes from 9 seconds to 40 milliseconds.

@BirdieMcFly

Hi @nippur72 ,

The change you made is very efficient!
However i think in the inject function you should test if the variable that accumulates css is not empty before touching the style DOM node. I think even when you append an empty string to existing css style DOM node, it forces the browser to interpret it again (which is the source of the lag) and i fear that if it's not checked there will be a latency on every mount() following the initial one (including mounts done using each).

@nippur72
Contributor

@BirdieMcFly yes, that makes sense, I've fixed it.

@BirdieMcFly

👍 nice

@GianlucaGuarini
Member

fixed in riot 2.3.13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment