guardian / frontend

The Guardian DotCom.
https://theguardian.com
Other
5.85k stars 556 forks source link

Snippets atom rendering is too slow #18841

Open mchv opened 6 years ago

mchv commented 6 years ago

When we implemented rendering of snippets atom with the atom renderer library we discussed how CSS integration could be done:

The solution we decided at that time, was to load the CSS client-side on demand using webpack.

The problem with the current solution is that CSS loading happens at the end (even after ads have rendered!) and the problem has been noticed and reported by both our users and editors who are unhappy with the current outcome.

Snippets atom content is fully part of the content, so its styling need to be applied at the same time than the style of the content.

Therefore I think we need to revisit the chosen solution.

When a piece of content is asked to be rendered, we know server side which atoms are part of the response (available in the Content API resource) so we could easily compute the CSS of atom types to include and include them either inlined or linked in the server response.

Steps to Reproduce

Open a page containing a snippet atom, for instance

Look at the atom style, wait and notice how long it take to be rendered correctly ⌚ ⏳

Actual Results

screen shot 2018-01-12 at 15 50 40

Expected Results

screen shot 2018-01-12 at 16 10 00

@sndrs @SiAdcock @nicl @NataliaLKB @regiskuckaertz

nicl commented 6 years ago

Okay, having tried this out, it is definitely a bad experience.

There are probably a few technical solutions to this which we should investigate. Only question I'd have though is whether we really need to consider atoms core to the content here? I think it is probably better to consider them add-on, as it gives us flexibility over how we render them etc. Yes, they are important, but it is also important for us to keep to a minimal strict definition of 'content' too.

regiskuckaertz commented 6 years ago

Add-ons, yes totally. But as first-class content, would you agree that they are more important than tweets or ads?

I fully agree, from the reader's perspective snippets (not atoms, as @paperboyo reminded me) are not the reason why they visit an article. From a journalistic POV, it is different though. They fill a very concrete gap in the journalist's utility belt, which is that he/she must constantly assume the reader doesn't know anything—where previously they would have had to write a separate article and lose the user, now they can embed an optional piece of journalism without loss of context. In short, snippets are the progressive enhancement of journalism 😅

paperboyo commented 6 years ago

My totally personal view: content or addons question ’sbit academic. Poor looks and poor experience is poor looks and poor experience. Late enhancement enforces view of them as auxillary too, so you get 🐔 & :egg:. Or is that :egg: & 🐔?

Media atoms OTOH definitely aren’t add-ons, right? But they render on fronts in a positively glacial tempo (while not yet using Regis’ lib AFAIK).

What am I saying here, cause I’m lost... Ah! Better than finding reasons not to load our own assets quickly... load them quickly, I guess.

chrismoranuk commented 6 years ago

Hi all. To be clear from the editorial side, snippets fit directly into Kath's purpose statement. They're a part of 'Be meaningful' and they are absolutely core journalism. That is the message we deliver directly to editors. They are a fundamental building block of our journalism...

Although I also agree with paperboyo that it's something of a moot point.

sndrs commented 6 years ago

The reason they're styled late is because is because we have to make trade-offs.

The more we inline the longer it takes before anything renders, so we need a very clear idea of priorities. Is it clear that snippets are more important than ads? If so, we can certainly move loading snippet assets up the chain.

We could also consider hiding them till their assets are loaded, since that includes any potential JS which will make those that need it unusable anyway.

That way, they will effectively lazy-load like rich links, most read etc.

The main reason we were (and I still am) pretty reluctant to inline their deps is that frontend has, by design, zero control over what they load.

As a department we're becoming increasingly fragmented and silo'd into teams with competing priorities (e.g. platform wants primary content rendered asap with minimal overhead; design wants as many fonts as possible – garnett alone has added 50% to the weight of the inline CSS on articles; you need the snippets rendered asap and commercial want ads on screen asap).

When everything is top priority then obviously nothing is...

As we lose the more holistic view that seemed prevalent a few years ago, frontend needs to build contingency and safeguards into how (what is effectively) 3rd party code runs on the site.

For example, what if an atom is created that is huge – how do we stop that affecting displaying content if it's inlined? It cannot be enough to say it won't happen: thrashers, video embeds already demonstrate that it will.

I can totally understand your problem thoguh, and we had begun work to obviate (or at least mitigate) the need for such sensitivity before we had to stop for garnett, and we're mega eager to pick it up again.

In the meantime, supposing we pushed loading atoms up the chain to the standard JS bundle, which runs before commercial, and atoms we're hidden by default then shown when their assets arrived? That would stop them ever displaying unstyled (and style them sooner), at the cost of delaying their appearance slightly?

nicl commented 6 years ago

@sndrs @chrismoranuk @paperboyo Alex has summarised this really well and makes my point better than I did.

The 'core' content thing isn't meant to be an academic exercise; we need to be brutal here because there are very big tradeoffs in terms of overall site performance and how much we display on initial load.

The solution of hiding them until assets are loaded seems like a clear win though.

mchv commented 6 years ago

Thanks @sndrs for you detailed and constructive response. Please find my comments below.

The more we inline the longer it takes before anything renders, so we need a very clear idea of priorities. Is it clear that snippets are more important than ads? If so, we can certainly move loading snippet assets up the chain.

yes snippets (I did not write all atom types) should load faster than ads if there is a need for a priority.

The main reason we were (and I still am) pretty reluctant to inline their deps is that frontend has, by design, zero control over what they load.

The css of each atom type is available in atom-renderer it can be reviewed and optimised. Each atom instance reuse the css here , there is no specific css per atom instance that can be added. By comparison:

In the meantime, supposing we pushed loading atoms up the chain to the standard JS bundle, which runs before commercial, and atoms we're hidden by default then shown when their assets arrived? That would stop them ever displaying unstyled (and style them sooner), at the cost of delaying their appearance slightly?

The solution of hiding them until assets are loaded seems like a clear win though.

hidden by default is not ok, as content (snippets atom are part of the content) is what should be returned and display first. If the CSS is hidden by default:

chrismoranuk commented 6 years ago

Yep - thank you all - Alex, I absolutely understand that we're talking about a variety of trade offs here and the suggested solution feels beneficial to me. Part of the user reaction I'm sure is related to that exposure followed by the delay in styling.

c

On 18 January 2018 at 13:04, Mariot Chauvin notifications@github.com wrote:

Thanks @sndrs https://github.com/sndrs for you detailed and constructive response. Please find my comments below.

The more we inline the longer it takes before anything renders, so we need a very clear idea of priorities. Is it clear that snippets are more important than ads? If so, we can certainly move loading snippet assets up the chain.

yes snippets (I did not write all atom types) should load faster than ads if there is a need for a priority.

The main reason we were (and I still am) pretty reluctant to inline their deps is that frontend has, by design, zero control over what they load.

The css of each atom type is available in atom-renderer https://github.com/guardian/atom-renderer it can be reviewed and optimised. Each atom instance reuse the css here , there is no specific css per atom instance that can be added. By comparison:

  • interactives atom: CSS is specific per each interactive and yet we inline it in the page!
  • thrasher: CSS is specific per each thrasher and yet we inline it in the page!
  • videos embed: CSS can be specific per each video and yet we inline it in the page!
  • CSS embed in composer: CSS is specific per each embed and yet we inline it in the page!

So in case we have no control a priori we inline, but in case of snippets, where we have control of the CSS we don't!

In the meantime, supposing we pushed loading atoms up the chain to the standard JS bundle, which runs before commercial, and atoms we're hidden by default then shown when their assets arrived? That would stop them ever displaying unstyled (and style them sooner), at the cost of delaying their appearance slightly?

The solution of hiding them until assets are loaded seems like a clear win though.

hidden by default is not ok, as content (snippets atom are part of the content) is what should be returned and display first. If the CSS is hidden by default:

  • It means content will move place at some point, which again is not a great user experience.
  • It means any js error, js loading delay, js unactivated will prevent the snippet content to be displayed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/guardian/frontend/issues/18841#issuecomment-358640472, or mute the thread https://github.com/notifications/unsubscribe-auth/AB-t3gIe7hAVIkg0zwodbqOyIEbpJNNsks5tL0FmgaJpZM4Rcgju .

--

Chris Moran

Editor strategic projects

Guardian News & Media


chris.moran@theguardian.com

@chrismoranuk https://twitter.com/chrismoranuk


Kings Place, 90 York Way, https://maps.google.com/?q=90+York+Way,%C2%A0+London+N1+9GU&entry=gmail&source=g

London N1 9GU https://maps.google.com/?q=90+York+Way,%C2%A0+London+N1+9GU&entry=gmail&source=g

theguardian.com

--


This e-mail and all attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender and delete the e-mail and all attachments immediately. Do not disclose the contents to another person. You may not use the information for any purpose, or store, or copy, it in any way. Guardian News & Media Limited is not liable for any computer viruses or other material transmitted with or as part of this e-mail. You should employ virus checking software.

Guardian News & Media Limited is a member of Guardian Media Group plc. Registered Office: PO Box 68164, Kings Place, 90 York Way, London, N1P 2AP. Registered in England Number 908396

GHaberis commented 6 years ago

@sndrs previous comment is a fair summary of the concerns we have to have within the dotcom platform team regarding maintaining performance objectives - we have seen performance hits from the use of interactive atoms, thrashers, video embeds so not wanting to repeat these is fair.

It sounds like hiding the atom by default until their assets have loaded is a no go, however can we not look at implementing part of the compromise put forward by @sndrs where we try pushing the loading of snippet atoms up the chain to the standard JS bundle so they take priority above Ads etc. I'd expect/hope that there'd be a noticeable improvement to snippet rendering times from doing this alone?

nicl commented 6 years ago

@mchv thanks for the reply. Completely agree re points relative to things like thrashers. I think those are areas we should seek to change the status quo too! There definitely isn't consistency here or a good overall approach (yet).

But bad status quo is not a good argument to potentially make things worse.

Having said that, the arguments about snippets being more important than ads and other things are compelling. And our approach should reflect that.

sndrs commented 6 years ago

i absolutely would not use the status quo as a precedent for what we should do now :) (esp below-the-radar hacks like thrashers)

The css of each atom type is available in atom-renderer it can be reviewed and optimised.

It's great that the code is public, but that does not mean that there willl always be a chance to review it.

It also does not mean that if we disagree about whether one particular change will be too deleterious to perf it wont be merged. Both of those ought to be true now and regularly are not (e.g. thrashers are merged without notification, JS polyfills for CSS are added for commercial deals that have already sold etc).

Both of these examples negatively impact performance and maintainability of dotcom in the medium to long term, but both achieve the immediate requirements of their teams.

What we need is a strategy that means you can satisfy your immediate requirements but will not affect the medium/long-term performance and maintainability of the site.

That is, in fact, exactly what we were working on before garnett waylaid us and will be again. I agree 100% we do not have that now (we never needed it before) and it looks like we'll need it more and more in the future.

Until then though, we need to be very wary.

Rather than asking "Are atoms important enough to render as quickly as the main content?", we could ask "Which features are sufficiently unimportant that rendering atoms can delay them?".

With that in mind, can we try moving the atom loading up the chain and see how it effects things? It's true it makes rendering more volatile than inlining, but it should run about 1s after first paint on low powered devices, and almost always below the fold (at least, this is what we were told previously?).

  • It means content will move place at some point, which again is not a great user experience.

this is true and i don't like it either. this is not an ideal solution.

  • It means any js error, js loading delay, js unactivated will prevent the snippet content to be displayed.

My understanding is these are supplemental to the main to content, is that fair? If it is, then prioritising loading seems reasonable, but prioritised below the main content.

It also has the valuable side-effect any atoms that rely on JS to function will not sit there rendered but inert if the JS fails.

regiskuckaertz commented 6 years ago

Sorry I closed by mistake 😅 Just released the update suggested by Georges and Alex, and it's much faster. See for yourself.