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

Lazy load components #3879

Merged
merged 21 commits into from
Jul 7, 2017
Merged

Lazy load components #3879

merged 21 commits into from
Jul 7, 2017

Conversation

sorin-davidoi
Copy link
Contributor

@sorin-davidoi sorin-davidoi commented Jun 20, 2017

Roadmap:

  • Display progress bar while fetching bundles
  • Routes
    • Lazy load (react-router guide)
    • Loading state
      • Empty column
      • Move styles to SCSS
    • Replay request (if bundle could not be fetched)
  • Columns:
    • Compose (+ reducer)
    • Home
    • Notifications (+ reducer)
    • Public timeline
    • Community timeline
    • Hashtag timeline
    • Loading state - empty column
  • Modals
    • Components:
      • Media
      • Onboarding
      • Video
      • Boost
      • Confirm
      • Report
    • Replay request (if bundle could not be fetched)
    • Move styles to SCSS
  • Do not show the waiting state if the bundle is fetched fast enough (~200ms)
  • Optimize bundles - perhaps tweak minChunks?
  • Measure size improvement

Bundles: https://gist.github.com/sorin-davidoi/07def6eb30df015fbfc8ced9fa36babd

@sorin-davidoi sorin-davidoi changed the title Lazy load components [WIP] Lazy load components Jun 20, 2017
@ykzts ykzts added performance Runtime performance ui Front-end, design work in progress Not to be merged, currently being worked on labels Jun 21, 2017
@sorin-davidoi sorin-davidoi changed the title [WIP] Lazy load components Lazy load components Jun 21, 2017
@sorin-davidoi
Copy link
Contributor Author

This reduces the size of application.js by 27% (673kB -> 485kb), while creating ~25 extra bundles. The progress bar is being used to show the download progress of each bundle. While the bundle is fetched, it displays a loading state in accordance with the content. If the fetch fails, an error component is shown, which provides a "refresh" button, allowing the user to replay the download.

@nolanlawson
Copy link
Contributor

nolanlawson commented Jun 21, 2017

Nice work! Based on the Webpack Bundle Analyzer output, though, it does appear that there are lots of common modules that are repeated throughout the async chunks – status.js, video_player.js, react-simple-dropdown, react-router-scroll, etc. I suppose WebpackCommonsChunkPlugin doesn't include those if they're included in async chunks? It seems useful to put those in common.js, given that there will probably be lots of users who load at least two of home/localTL/federatedTL/accountTL/hashtagTL/etc.

@nightpool
Copy link
Member

nightpool commented Jun 22, 2017

I'm not sure I see the practical benefit of this—won't it just increase latency on poor connections due to the extra round trip requests?

Is there a way to measure whether the decreased bundle size will actually reflect real world usage?

@sorin-davidoi
Copy link
Contributor Author

It should decrease time to first paint (less JavaScript to parse, which is the main bottleneck on mobile), as well as memory usage. Don't think we are currently measuring those?

@sorin-davidoi
Copy link
Contributor Author

As a side note, it would be nice to set up Lighthouse integration, to measure these metrics for each PR.

@sorin-davidoi
Copy link
Contributor Author

Unscientific findings (running Lighthouse against current master and this branch):

  • Performance score: +3 (51 -> 54)
  • Render blocking scripts: -200ms (2070ms -> 1820ms)
  • First paint: -140ms (7060ms -> 6950ms)
  • First interactive: -600ms (7570ms -> 6950ms)

I would prefer to take the average of a few tests but I don't have the time to do it (is there a tool for it?).

@nolanlawson
Copy link
Contributor

won't it just increase latency on poor connections due to the extra round trip requests?

That can be fixed by adding <link rel=preload>s for each of the chunks that we expect are very likely to get loaded by the user. I think this would be a smart addition; @sorin-davidoi you can add it to application.html.haml like so:

%link{:rel => "preload", :as => "script", :src => (asset_pack_path 'application.js')}/

Also for your Lighthouse score, what page did you test against? There are a few I think we want to prioritize:

  • /web/getting-started – main home
  • /web/timelines/home – mobile PWA home (defined in manifest.json)

I also imagine there are people who bookmark local/federated/home timelines, so those would be worthwhile to preload as well.

Overall, I believe it is worthwhile to only load those routes that the user explicitly requested, since it limits overhead of JS parse/exec (and also memory). But avoiding extra network roundtrips is important, and also I'd like to see some deduplication of the shared modules across async chunks.

@sorin-davidoi
Copy link
Contributor Author

I tested the local timeline (mobile UI). <link rel=preload> sounds nice, will try to add them! 👍

@sorin-davidoi
Copy link
Contributor Author

sorin-davidoi commented Jun 22, 2017

I think we could also lazy-load some reducers (see https://stackoverflow.com/questions/32968016/how-to-dynamically-load-reducers-for-code-splitting-in-a-redux-application), but that is a can of worms I don't want to open for now. Proceeds to open can of worms.

@beatrix-bitrot
Copy link
Contributor

Is this ready for review?

@sorin-davidoi
Copy link
Contributor Author

sorin-davidoi commented Jun 22, 2017

Just a few final touches to do. Found a crash as well.

@sorin-davidoi
Copy link
Contributor Author

sorin-davidoi commented Jun 22, 2017

I'd say it is now ready for review. Could lazy-load more reducers I think but I would rather do it in another PR. Would also be nice to lazy-load the Emoji picker using the same idioms used here, but that can wait as well.

@beatrix-bitrot
Copy link
Contributor

nice, because it's a big PR and not my area of expertise I will test this branch on glitch.social to make my review

Copy link
Contributor

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

Codewise this looks great to me; I just have a few small suggestions. This is also deployed at malfunctioniong.technology (with service worker as well) for those who want to take a look.


// Dummy import, to make sure that <Status /> ends up in the application bundle.
// Without this it ends up in ~8 very commonly used bundles.
import '../../components/status';
Copy link
Contributor

Choose a reason for hiding this comment

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

nice fix


= javascript_pack_tag 'features/community_timeline', integrity: true, crossorigin: 'anonymous', rel: 'preload'

= javascript_pack_tag 'features/public_timeline', integrity: true, crossorigin: 'anonymous', rel: 'preload'
Copy link
Contributor

Choose a reason for hiding this comment

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

These should have as="script" as well; it's explained what the browser does with this extra information here.

@nolanlawson
Copy link
Contributor

In terms of bundle sizes this also looks great. I rebuilt and put a Webpack Bundle Analyzer view here for those who want to see. (Protip: make a Gist with an index.html and bl.ocks.org can host it for you. 😃)

@Gargron
Copy link
Member

Gargron commented Jun 25, 2017

@nolanlawson Waiting on you to check the changes.

@akihikodaki akihikodaki removed the work in progress Not to be merged, currently being worked on label Jun 27, 2017
Copy link
Contributor

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

This looks great to me, and is live on malfunctioning.technology to test. (You may need to hard-refresh due to ServiceWorker.)

We may want to warn admins when we ship this that they will probably want to enable HTTP/2 since we are now preloading 13 JS files, which will be slow on HTTP/1 due to the 6-connections-per-origin limit. We may also want to tweak the preloading to limit it to only those components that are guaranteed to be loaded (e.g. "getting started" will only be loaded for first-time users; maybe we can detect that somehow with cookies?).

But those are minor concerns; overall this looks awesome to me and I'm 👍 to merge. I especially love the new async-components.js so we can keep all of those components in one place. Great work!

@nolanlawson
Copy link
Contributor

BTW http2 is recommended in the tootsuite/documentation nginx config so hopefully most admins already have it.

@unarist
Copy link
Contributor

unarist commented Jun 30, 2017

Component which is loaded before also seem to be lazy loaded.

image

I think it can be avoided by caching result of import(). (sample: unarist@4cf1fde)

image

Pros: avoid flashing of loading view, slightly reduces total time
Cons: single script execution will be longer, i.e. blocks browser longer

How about this?

@sorin-davidoi sorin-davidoi mentioned this pull request Jul 7, 2017
2 tasks
@Gargron Gargron merged commit 348d6f5 into mastodon:master Jul 7, 2017
@Gargron
Copy link
Member

Gargron commented Jul 8, 2017

@sorin-davidoi Please guide me through where the "loading" and "error" views are defined and how to get to see them without a slow connection so I can make sure I like how they look visually.

@sorin-davidoi
Copy link
Contributor Author

@Gargron Open the Network tab in the Chrome development tools and tick "Disable cache".

For columns:

  • Loading state - Load a timeline, throttle the connection to GPRS, click on a toot.
  • Error state - Load a timeline, tick the "Offline" checkbox, click on a toot. After the error state is shown you can uncheck "Offline" and click retry, the normal page should appear.

For modals:

  • Loading state - throttle the connection to GPRS and open a media modal
  • Error state - tick "Offline" and open a media modal - same behavior with retry

@sorin-davidoi sorin-davidoi deleted the lazy-loading branch July 8, 2017 09:13
rtucker pushed a commit to vulpineclub/mastodon that referenced this pull request Jul 8, 2017
* feat: Lazy-load routes

* feat: Lazy-load modals

* feat: Lazy-load columns

* refactor: Simplify Bundle API

* feat: Optimize bundles

* feat: Prevent flashing the waiting state

* feat: Preload commonly used bundles

* feat: Lazy load Compose reducers

* feat: Lazy load Notifications reducer

* refactor: Move all dynamic imports into one file

* fix: Minor bugs

* fix: Manually hydrate the lazy-loaded reducers

* refactor: Move all dynamic imports to async-components

* fix: Loading modal style

* refactor: Avoid converting the raw state for each lazy hydration

* refactor: Remove unused component

* refactor: Maintain modal name

* fix: Add as=script to preload link

* chore: Fix lint error

* fix(components/bundle): Check if timestamp is set when computing elapsed

* fix: Load compose reducers for the onboarding modal
Gargron pushed a commit that referenced this pull request Jul 10, 2017
* Add Japanese translations for #3879

* Add Japanese translations for #4033

* Add Japanese translations for #4136
rtucker pushed a commit to vulpineclub/mastodon that referenced this pull request Jul 10, 2017
* feat: Lazy-load routes

* feat: Lazy-load modals

* feat: Lazy-load columns

* refactor: Simplify Bundle API

* feat: Optimize bundles

* feat: Prevent flashing the waiting state

* feat: Preload commonly used bundles

* feat: Lazy load Compose reducers

* feat: Lazy load Notifications reducer

* refactor: Move all dynamic imports into one file

* fix: Minor bugs

* fix: Manually hydrate the lazy-loaded reducers

* refactor: Move all dynamic imports to async-components

* fix: Loading modal style

* refactor: Avoid converting the raw state for each lazy hydration

* refactor: Remove unused component

* refactor: Maintain modal name

* fix: Add as=script to preload link

* chore: Fix lint error

* fix(components/bundle): Check if timestamp is set when computing elapsed

* fix: Load compose reducers for the onboarding modal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Runtime performance ui Front-end, design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants