Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EPIC: xPRO performance improvements #2065

Closed
3 tasks
briangrossman opened this issue Jan 13, 2021 · 14 comments
Closed
3 tasks

EPIC: xPRO performance improvements #2065

briangrossman opened this issue Jan 13, 2021 · 14 comments
Assignees
Labels

Comments

@briangrossman
Copy link
Contributor

briangrossman commented Jan 13, 2021

As a user, I am experiencing slow load times on the xPRO site. The slow load time may cause me to leave the site, before I can find anything I am looking for. It is particularly noticeable on mobile.

Designs and Mockups

To test, use the Lighthouse tab in Chrome's developer tools. The image below is a sample output for the homepage on mobile

Acceptance Criteria:

This EPIC includes the following issues:

image

@HamzaIbnFarooq
Copy link
Contributor

@arslanashraf7 and I worked on xPro's Performance issue today and identified these initial points that are needed for optimization:

Requires Less Effort:

  1. We are mostly using .PNG images, whereas it's recommended to use WebP images instead.
  2. Define inline-image sizes, where possible to reduce layout shifts.

Requires More Effort

  1. Query optimizations
  2. Defer/Async all possible third-party JS files e.g: Zendesk

Need More Insights

  1. Compression of payload
  2. Reduce JS execution time (most probably on base.html)
  3. Enhance caching
  4. Devise a way to load selective css and js for different pages

Most of the changes will be done in base templates, which might be enhancing overall performance including catalog, product, and home pages.

cc: @pdpinch @briangrossman

@HamzaIbnFarooq
Copy link
Contributor

After spending more time on this issue @arslanashraf7 and I came to the conclusion that the maximum blocking time is taken by JS bundle downloads while ~50% of that JS is not even getting used. According to our findings, we are currently using django.js, header.js, third-party.js bundles, compressed via webpack.

In the light of the above explanation, we propose the following two solutions for that:

  1. Division of webpack bundles:
    For example: django.js's bundle contains all the django related JS files (both important [required immediately] and unimportant [which can be deferred]) and is getting loaded in our base template, so we can't separately download unimportant JS files asyncronously. So if we divide django.js then we can decrease the JS load time.
  2. Using React Lazy loading:
    Lazy and Suspense are two relatively new features of reactjs, they seem to be quite promising for lazy loading react component but might require some intense refactoring of react code. It might help us reducing the size of header.js and other react specific bundles.

@pdpinch @briangrossman any thoughts on these solutions?

@briangrossman
Copy link
Contributor Author

@HamzaIbnFarooq - This is great start. I agree that the JavaScript seems to be at the core of this issue. I don't have a personal opinion (or enough working knowledge, rather) to suggest which path to take for breaking up the JavaScript.

I've reached out to @gsidebo, who will be helping support your efforts here. He'll weigh in with his thoughts as you work through these issues.

Thanks!

@gsidebo
Copy link
Contributor

gsidebo commented Jan 19, 2021

@HamzaIbnFarooq @arslanashraf7 I'm going to try to go through those specific suggestions point-by-point, but I have some scattered thoughts to consider in the meantime:

  1. We should definitely try requesting all of our desired Google fonts in a single <link> tag. Apparently they just need to be separated by a pipe character (doc)

  2. I definitely agree with setting heights on certain elements while we wait on images to load. This is particularly important with the header. The very least we should do is set a height for some container element so that when the header contents load, the content below it does not shift. We may even want to consider moving the "MIT | xPRO" logo outside of the bundle and into an HTML template. If needed, we could also make that optional via a prop (in other words, only include the logo & link inside the javascript-based header if a certain prop is passed in)

  3. There are definitely a bunch of scripts loaded in third-party.js that can be deferred until after content loads. popper.js, slick-carousel, and @fancyapps/fancybox for sure. Maybe jquery and some others as well.

  4. In django.js, there are a bunch of scripts bundled up, some of them use $(document).ready() to delay execution, some of them just execute right away. I don't believe there is any guarantee that jQuery would even be loaded when those scripts are executed. I think we might want to try the following:

    1. Change all of those scripts so that they don't run anything right away, but instead they export a function that does all of the relevant work.
    2. In django.js, call each one of those exported functions in series inside of a listener: document.addEventListener("DOMContentLoaded", function(event){ ... })
    3. Make sure django.js only executes after jQuery is loaded. To accomplish this, I think we can just load third-party.js and django.js both with defer attributes, and make sure the django script is loaded second
  5. I'm seeing a ton of Zendesk imports. Can we make sure those are actually necessary?

    ss 2021-01-19 at 18 39 16

  6. We may want to look into this lazy loading images plugin for Wagtail: https://pypi.org/project/wagtail-lazyimages/

@HamzaIbnFarooq
Copy link
Contributor

HamzaIbnFarooq commented Jan 20, 2021

Thanks, @gsidebo for mentioning the detailed insights. Here are our thoughts on this:

  1. We should definitely try requesting all of our desired Google fonts in a single <link> tag. Apparently they just need to be separated by a pipe character (doc)

We have already covered this step (#2073)

  1. I definitely agree with setting heights on certain elements while we wait on images to load. This is particularly important with the header. The very least we should do is set a height for some container element so that when the header contents load, the content below it does not shift. We may even want to consider moving the "MIT | xPRO" logo outside of the bundle and into an HTML template. If needed, we could also make that optional via a prop (in other words, only include the logo & link inside the javascript-based header if a certain prop is passed in)

We have added image sizes partially (#2073) but it requires further in-depth work.

  1. There are definitely a bunch of scripts loaded in third-party.js that can be deferred until after content loads. popper.js, slick-carousel, and @fancyapps/fancybox for sure. Maybe jquery and some others as well.

Strongly agreed.

  1. In django.js, there are a bunch of scripts bundled up, some of them use $(document).ready() to delay execution, some of them just execute right away. I don't believe there is any guarantee that jQuery would even be loaded when those scripts are executed. I think we might want to try the following:

    1. Change all of those scripts so that they don't run anything right away, but instead they export a function that does all of the relevant work.
    2. In django.js, call each one of those exported functions in series inside of a listener: document.addEventListener("DOMContentLoaded", function(event){ ... })
    3. Make sure django.js only executes after jQuery is loaded. To accomplish this, I think we can just load third-party.js and django.js both with defer attributes, and make sure the django script is loaded second

Yes, you are right, it shows a huge potential for performance improvement.

  1. I'm seeing a ton of Zendesk imports. Can we make sure those are actually necessary?

Yes, these are necessary for the rendering of zendesk help button, we have already deferred (#2072), so it should not be a problem anymore.

  1. We may want to look into this lazy loading images plugin for Wagtail: https://pypi.org/project/wagtail-lazyimages/

For now, we have used HTML's loading='lazy' feature for static image loading (#2073), but we must have a look into wagtail-lazyimages library as it generates a thumbnail image before loading the whole image.

cc: @arslanashraf7

@HamzaIbnFarooq
Copy link
Contributor

@gsidebo @arslanashraf7
Point 3, 4, and 5 are partially covered in #2072, so kindly have a look into it.

@HamzaIbnFarooq
Copy link
Contributor

  1. We may want to look into this lazy loading images plugin for Wagtail: https://pypi.org/project/wagtail-lazyimages/

Wagtail-lazyimages looks pretty good but has two limitations to be addressed:

  1. It doesn't support static assets loading (e.g: {% static 'images/mit-dome.png' %})
  2. We have a custom tag (image_version_url) which will not be supported by it.

So we can use the plugin but, in my opinion, we need to create a new wrapper tag and override one of the plugin's method (wagtail_lazyimages.templatetags.lazyimages_tags.LazyImageNode.render) to support our scenarios.

cc: @gsidebo @arslanashraf7

@gsidebo
Copy link
Contributor

gsidebo commented Jan 21, 2021

@HamzaIbnFarooq Yeah, I was expecting we would have to write a new templatetag to support this plugin. I don't know the implementation details but I'm not expecting it to be too difficult. Re: static, I think it's fine to leave those as they are now. Just as a first step we can consider styling those images (or their container elements) with heights/widths that will prevent the content from shifting after it loads

@gsidebo
Copy link
Contributor

gsidebo commented Jan 22, 2021

After reviewing where we're at, I think our highest priorities should be these items:

  1. Evaluate the lazy image loading plugin (currently in progress: add image lazy loading using wagtail-lazyimages plugin #2081)
  2. Combine django.js and third_party.js. Unless I'm mistaken, I don't think those actually need to be separate script files. As far as I can tell they are both used in base.html and nowhere else.
  3. Investigate eliminating font-awesome and icomoon entirely. These aren't huge requests, but our usage of both of those libraries seems low enough that we should see if we can replace them with elements from material-icons etc.
  4. Figure out what's going on with Zendesk, Hubspot, Google Analytics/Tag Manager, and potentially other 3rd party libraries. When testing on RC with simulated 3G connection, I am seeing assets being loaded from those vendors well before the first content paint. This is particularly concerning in the case of Zendesk, as loading the home page fires off something like 10 requests to them. Their script/stylesheets imports are running too soon, our scripts that fill in the content are running too late, or both.
  5. Deep dive into reducing the sizes of our header bundle and possibly our style bundle. The header bundle on production is 266kb and is the largest download by far after the .ts videos and the youtube player script. This will require some close analysis and brushing up on webpack best practices, and it's difficult to predict the gains we can make.

EDIT: One other thing we should do is move the contents of the <script> in product_page.html into django.js. Not every marketing page needs this code, but it's a small enough amount of code that the added loading time will be very low, and it significantly reduces the risk of issues like we experienced today (#2083)

@arslanashraf7
Copy link
Contributor

arslanashraf7 commented Jan 25, 2021

@gsidebo I investigated around

Investigate eliminating font-awesome and icomoon entirely. These aren't huge requests, but our usage of both of those libraries seems low enough that we should see if we can replace them with elements from material-icons etc.

And it seems we can almost replace icomoon and font-awesome with Google's Material Icons but there are few i couldn't find in Google Material Icon palette https://material.io/resources/icons/?style=baseline.
For the icons available with Google Material Icons, We might see a very slight look & feel difference (edges/sharpness).
If we decide to move along with this change we might need to create a way out for the icons that are not available in Google Material Icons.
Im mentioning the usage of both and their replacement possibilities in the below list.

Font-Awesome Icons:

  1. fa-chevron-down (Replaceable)
  2. fa-chevraon-up (Replaceable)
  3. fa-facebook-square (Replaceable but available in round border - https://material.io/resources/icons/?search=facebook&icon=facebook&style=baseline)
  4. fa-linkedin (Not available with Google Material Icons)
  5. fa-twitter (Not available with Google Material Icons)

Unused FA-Icons:

  1. fa-chevron-left (Not Used in our application)

Icomoon Icons:

  1. icon-close-outline (Replaceable)
  2. icon-arrow-circle-up (Replaceable)
  3. icon-chevron-left (Replaceable)
  4. icon-chevron-right (Replaceable)
  5. icon-arrow-circle-right (Replaceable but no outline circle in the icon from google )

Unused Icomoon icons:

  1. icon-arrow-circle-left (Not Using)
  2. icon-arrow-circle-down (Not Using)

CC: @briangrossman You might also want to take a look on these details, Since we had an option in mind to replace icomoon & font-awesome icons packages to improve the performance. Feel free to ask/add any details that you see relevant which i might have overlooked.

@arslanashraf7
Copy link
Contributor

arslanashraf7 commented Jan 25, 2021

@gsidebo I've created a PR #2086, That specifically addresses two points from your above comment #2065 (comment):

  1. Combine django.js and third_party.js. Unless I'm mistaken, I don't think those actually need to be separate script files. As far as I can tell they are both used in base.html and nowhere else.

As for point#2 you were right, these were only used in base.html

EDIT: One other thing we should do is move the contents of the <script> in product_page.html into django.js. Not every marketing page needs this code, but it's a small enough amount of code that the added loading time will be very low, and it significantly reduces the risk of issues like we experienced today (#2083)

@gsidebo
Copy link
Contributor

gsidebo commented Jan 28, 2021

I mentioned this on the PR, but the wagtail lazy images PR carries some risk for us (comment), so for now I'm suggesting that we put it on hold. Delaying image load is still a priority for us, though. Images are responsible for a lot of the load time. We may just want to try implementing Observer-based lazy loading without that Wagtail plugin.

Some other things we should look at now:

  • Set a height on the <div id="header"> or the <header> element to get rid of the significant content shift when contents of the header are loaded in via script.

  • Try getting rid of font-awesome. I'm no SVG expert, but I think that we should be able to recreate the look of the social icons as they currently exist in the certificates page using our own styles. As for the chevrons, it seems like we can replace them more or less directly with the material icons equivalent. Even if we can't replace the social icons for whatever reason, we can replace the chevrons, then move the font-awesome import to the certificates page, which is the only page that uses the font-awesome social icon styling. The minified font-awesome style import in 6.8kb. Not gigantic but on fast 3G that's about 1.75s of loading time that seems easy enough to eliminate.

  • It would be great to get rid of icomoon too. The arrow-circle-right icon that @arslanashraf7 mentioned seems like it's only used in the hubspot form at the bottom of the page (see screenshot). I don't think it will be a problem to replace that with a different-looking arrow. We can tag Abdul in the PR with before/after screenshots. I'd say this is lower priority than font-awesome, but still worth the effort.

    ss 2021-01-27 at 19 28 49

@arslanashraf7
Copy link
Contributor

@gsidebo , As per your above comment #2065 (comment), I was able to completely remove Font-Awesome & Icomoon with Google Material Icons though with slight look and feel changes e.g sharpness of icons/or outlines.

Also i didn't need to replace SVGs, they weren't actually dependent on Font-Awesome which i mentioned earlier, So we are good there.

As for setting the height of Header, I tried to do that but that eventually effects the responsiveness of the header, So instead i just set the height to TopAppBar which was feasible ATM.

The PR for these changes is up at #2093

@gsidebo
Copy link
Contributor

gsidebo commented Dec 2, 2021

There's nothing from this epic that needs to be addressed in the near term. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants