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

frontend speed improvements - cuts landing page load time by 55% and DOMready time by 35% #4791

Closed
wants to merge 13 commits into from

Conversation

owocki
Copy link
Contributor

@owocki owocki commented Jul 12, 2019

Description

up until now, my efforts on site speed have been mostly oriented around HTML generation time. nwo that we've gotten the HTML gen time down, i'm turning my efforts towards focusing more on front-end optimizations.

this is a PR that takes the landing page load time on gitcoin from

Screen Shot 2019-07-12 at 5 39 35 PM

to

Screen Shot 2019-07-12 at 5 35 14 PM

on the intial page paint, thats:

  • 146 to 30 requests
  • 4.8MB to 2.2MB in total page size
  • 1.88s to DOMReady to 1.2s
  • 2.5s to Load to 1.57s

on a cold cache.

on a warmed cache itll take us from

  • (same requests)
  • (same size)
  • 930ms to DOMReady to 730ms
  • 1.65s to Load to 1.14s

how

  • uses django-compress to combine all the css and js files on a page into one http request response
  • uses jquery-unveil to defer any below-the-fold images until after the user scrolls down
  • uses AWS_HEADERS to set far future expires headers on AWS
  • uses django-statici18n to serve the i18n information along with the script js (instead of being another XHTTP request that blocked painting of text)
  • removes old trackcing scripts that we dont use any more (facebook + twitter pixel)
  • compressing all the images on the landing page ( these images are uploaded to https://github.com/gitcoinco/web/tree/kevin/site-perf2 ; there are 1000s of them, so i did not included them in this PR)

further TODOs

  • many of these things are just done on the landing page, and will need to be applied across the site too.
Testing

tested landing page locally

@owocki owocki changed the title cuts landing page load time by 55% and DOMready time by 35% frontend speed improvements - cuts landing page load time by 55% and DOMready time by 35% Jul 12, 2019
@owocki owocki force-pushed the kevin/site_perf branch from 541c521 to 1437c9e Compare July 13, 2019 20:20
@owocki
Copy link
Contributor Author

owocki commented Jul 13, 2019

if/when we merge this we should also merge https://github.com/gitcoinco/web/tree/kevin/site-perf2

@owocki
Copy link
Contributor Author

owocki commented Jul 15, 2019

@danlipert feedback?

@@ -224,7 +224,7 @@ img.play {
cursor: pointer;
--padding: 40px;
background-color: var(--gc-green-hover);
background-image: url('../images/home/bg-video-preview.svg');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this allows us to load them inline upon scroll instead of on pageload.

@@ -0,0 +1,6 @@
/*!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved the medium images and vue.js images to our own site so they could be cached according to our CDN config..

@@ -441,8 +441,8 @@ <h5 class="bounty-heading">{% trans "Funder" %}</h5>
<script src="/dynamic/js/tokens_dynamic.js"></script>
<script src="{% static "v2/js/tokens.js" %}"></script>
<script src="{% static "v2/js/amounts.js" %}"></script>
<script src="{% static "v2/js/lib/popper.min.js" %}"></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in base template now

Copy link
Contributor

@octavioamu octavioamu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments

<script src="https://cdn.jsdelivr.net/npm/vue/dist/vue.js"></script>
{% endif %}
{% include 'shared/sentry.html' %}
<script src="{% static "v2/js/lib/vue.js" %}"></script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add <script src="https://cdn.jsdelivr.net/npm/vue"></script> to not prod environment ? that is used to debug, so will show the correct vue errors in localhost

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -29,8 +29,8 @@
<script src="https://cdn.jsdelivr.net/npm/vue/dist/vue.js"></script>
{% endif %}
{% include 'shared/sentry.html' %}
<script src="{% static "v2/js/lib/popper.min.js" %}"></script>
<script src="{% static "v2/js/lib/bootstrap.min.js" %}" crossorigin="anonymous"></script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to move bootstrap.js to footer_scripts and footer_scripts_lite since now is used in most parts and is part of the framework.
I had changed that in this PR #4727

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it - maybe we should just have a seperate compress section here..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes for sure we can add it later. In some point we will get in a place we can just use the "lite" version and deprecate the other, but need to check since there are diff parts of the site with diff needs and for example the shared.js is something Im trying to avoid to be loaded in all pages. Maybe having one compress for footer_scripts_lite and one for footer_scripts

{% include 'shared/tag_manager_1.html' %}
<meta charset="utf-8">
{% compress css %}
<link rel="preload" href="{% static "v2/css/base.css" %}" as="style">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a preload tag, so will try to find that file in the server to pre fetch it. Maybe since will be now a big css file, it should preload that file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess it depends on if we want it to block the loading of the rest of the site..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will create one file name.css right? I mean <link rel="preload" href="{% static "v2/css/base.css" %}" as="style"> is not the actual call of the file just the instruction to prefetch it. I guess we should have in compress just the files that are loaded in all pages and then add one <link rel="preload" href="{% static "v2/css/compress.css" %}" as="style">

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will create one file name.css right?

yes

I guess we should have in compress just the files that are loaded in all pages and then add one

i read the docs on this and i dont understand what gain this gives the site.. CSS loading should not block the rendering of theDOM right? AFAIK it happens ineline

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I think is not about blocking is about kb that will load minification + concat. For other side I think the most valuable for us is the minification part since we use http2 and the conections to fetch the diff files will be at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the preload what happens is the browser knows the priority of the side and will load as soon as possible avoiding any repaint in the browser side

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is a good article showing examples of use of preload https://medium.com/reloading/preload-prefetch-and-priorities-in-chrome-776165961bbf

<link rel="stylesheet" href="{% static "v2/css/jquery.modal.min.css" %}" />
{% endif %}
<link rel="stylesheet" href="{% static "v2/css/jquery-ui.css" %}" />
<link rel="stylesheet" href="{% static "v2/css/jquery.modal.min.css" %}" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding modal and jquery-ui to all the pages will break a lot of pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does it break? wouldnt pages without a modal / jquery-ui just not activate the features in this css files?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jquery-ui modals styles overwrite bootstrap ones because they use general class like .modal without descendent of special class. There left just a few places where we using jquery-ui the ideas is totally remove it. I think if you open any modal like submit work you will get a messed modal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it - is there a task to remove the jquery-ui dependancies? id add some bounty funds to it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if there are other features of jquery ui we are using...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just modals and tooltips

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@octavioamu
Copy link
Contributor

Some questions
how compressor works?
it runs in dev also?
We can control the generated file name?

@octavioamu
Copy link
Contributor

Some questions
how compressor works?
it runs in dev also?
We can control the generated file name?

Is this the doc? https://django-compressor.readthedocs.io/en/stable/usage/

@owocki
Copy link
Contributor Author

owocki commented Jul 15, 2019 via email

@danlipert danlipert mentioned this pull request Jul 16, 2019
@danlipert
Copy link
Contributor

PR for images: #4800

@owocki
Copy link
Contributor Author

owocki commented Jul 16, 2019

just addressed everyones comments.

@owocki
Copy link
Contributor Author

owocki commented Jul 17, 2019

just pushed up one commit.. i think i got evertything

@owocki
Copy link
Contributor Author

owocki commented Jul 18, 2019

any other comments? im going to resolve the merge conflicts now

i want to apply this to other pages that take too long to load because of frontend issues next.. as long as the group thinks we are going in a directionally correct way

@owocki
Copy link
Contributor Author

owocki commented Jul 18, 2019

actually, i probably will just cherry-pick the changes into a new PR with a clean commit history rather than resolve the conflicts here..

@owocki
Copy link
Contributor Author

owocki commented Jul 18, 2019

new PR #4826

@thelostone-mc
Copy link
Member

Closing this out as this is a dupe of #4826

@thelostone-mc thelostone-mc deleted the kevin/site_perf branch June 27, 2020 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants