Perf audit: Loading performance #247

Open
paulirish opened this Issue Jul 2, 2015 · 52 comments

Projects

None yet
@paulirish

Been meaning to do a performance audit of the new mobile site. I'm going to drop it right here into the ticket. I may add comments as I go along.

Initial Test: Loading (total duration: 45 seconds)

Test setup: 2013 Moto X. On 4G (HSPA+). Chrome Dev Channel.

  1. Load reddit.com
  2. Tap on the big blue link to load the mobile site.
  3. Wait until the site feels loaded.

Expectation: Site should load and give me the frontpage items and images in < 5 seconds.
Result: Site came up with items and image placeholders within 3 seconds. It took 45 seconds to show images.

Here's the overview of Timeline and and network activity.

Timeline. (click to zoom)

image

Network waterfall, annotated.

image

@paulirish paulirish changed the title from Loading performance to Loading performance: 45 seconds to load Jul 2, 2015
@paulirish

During all that time, the page is scrolling just fine. It feels probably interactive, and as a user I'm wondering why the images are taking so long.

However if during that time i click on a link to read comments I will wait up to 16 seconds for that tap to be handled.

screenshot of the kind of thing i'm looking at:

So.. the next question is.. what's the holdup?
Let's find out. :)

@paulirish

Basically, a megabyte of javascript.
Indeed, client.min.js is 1.1MB of minified code.

While scrolling is responsive, the main thread is jam packed, having a party.

Overall timeline

Here's a look at what happens from start to finish of that 45 seconds:

image

Shareable timeline

You can open this recording up to zoom in and poke around. Open this crazy-ass link in Chrome stable:
chrome-devtools://devtools/bundled/devtools.html?remoteFrontendUrl=chrome-devtools://devtools/remote/serve_rev/@198240/inspector.html&loadTimelineFromURL=http://temp.paulirish.com/timeline-new-reddit-load-long-wait-for-images.json

Timeline in 4 Acts

We can break up all that work into 4 sections for better inspection:
image

From a quick glance here's what each act contains:

  • Act 1: Skeleton (4 seconds): Initial HTML, css, JS downloaded. Skeleton DOM is built. Everything is laid out.
  • Act 2: Bootup (13 seconds): Bootup and initialization of Polyfills, Lodash, horse react, html parser, moment.js via formatdifference, greensock/gsap, and a few components.
  • Act 3: Component Party (22 seconds): React is mounting components like the dickens. So much mount. Right after a brief pause, there's another 3 seconds where it's updating all components due to an invalidation. It's right in that pause where the network requests for the images are kicked off.
  • Act 4: Resize Twice (7 Seconds): The browser's resize event fires. Twice, for some reason. Not sure why why, but the handler takes ~3s each time to run.

Around the time those resize handlers finish the network requests for the images complete and we get what we came for!

@paulirish

Initial Impressions:

  • The network requests should start much earlier. <img> tags in the initial HTML would mean the page is pretty much fully visually rendered at about 4 seconds in.
  • There's way too much javascript here. While the development stack (babel, react, npm, etc) is modern and feels great, it ends up shoving a lot of JS at the browser to make it all work.
  • Based on the fact that I do see the skeleton at the start, it feels like you're already doing some serverside rendering, yeah? Unfortunately the lazy initialization is quite costly.
  • The long non-interruptible JS blocks are a problem. (Details following…)

Main thread JS & Input.

Chrome & Safari will prioritize input handlers over other javascript. Imagine if you had 10 handlers on dom-ready and they all took a long time, but during that time you tap a button with a click handler. Instead of waiting for the queue of all JS to finish, the browser can sneak in touch/click handlers early so that the user gets an update faster. Input latency reduction ftw. (Description & video: http://blog.chromium.org/2015/04/scheduling-tasks-intelligently-for_30.html )

However in this case, because we have 2 very large non-interruptible blocks of JavaScript, the browser cannot respond to anything the user does that requires JavaScript to run until those blocks complete. Since everything in this UI needs JavaScript, including clicking on the story link, it's all held up.

Image network requests

The images on the stream are done via css background images, but they aren't in place in the original HTML. (Assuming for lazyload purposes?) I'm not sure when the inline styles are placed, but just applying those styles won't force the requests to start, as there can be an !important rule elsewhere that's overriding them. Since they're CSS we have to wait till the next Recalculate Styles operation. Once that happens, then the browser will know for sure that we can use them. In our case, that doesnt happen until later when ListingContent triggers a GSAP animation, which asks for the offsetHeight of an element, forcing a layout/reflow, and that needs style calculation:

image

@paulirish

Since there's a sea of JS here, let's try to parse it into our top costs.

This is an experiment in DevTools showing a flame chart of all costs aggregated... more like a classic flame graph:
image

If we zoom in there are a few recognizable chunks at the top:

  • .2.5s of layout thrashing coming from s.value..
  • this TextSubNav kickoff work that's pretty hefty:
  • react mountcomponent costs

But to be honest, I'm not getting a lot out of this particular view, so lets try something else.

@paulirish

I took the browserify bundle that gulp finishes with and tossed it at disc and now we can get a better understanding of what's consuming space in our bundle.

Open the visualization

image

Edit: I've re-run the disc using the minified output and updated the above URL. The unminified results are here where you can see lo-dash consuming more space than you'd expect…

@paulirish

Impressions from disc bundle sunburst

  • greensock is pretty huge. (100kb min'd). I did see it get triggered during load (in script) but i've never seen a smooth animation due to the other main thread activity going on.
  • dom-serializer is included in the bundle (findable in the production client.js), but it's a devDependency, so i think that's a mistake.
  • sanitize-html is fairly big (63kb min'd), but you probably need it. :) One opportunity I'm thinking though…
  • lodash is included in both sanitize-html and as a regular dependency. It'd be great to dedupe those.
  • babel ships with bunch of polyfills. I can't tell if those are all being used in the application, but it'd be valuable to cull the unused ones. The biggest is es5.js which is 4.5kb min'd and likely unneccessary for most browsers hitting m.reddit.

Impact

This is mostly just a sweep for sanity. Addressing the items above will drop the filesize of the payload, which is certainly worthwhile. The impact on overall loadtime perf is a little harder to suss out. It appears these changes might wipe out up to 1/3 of the time spent in Act 2: Bootup, but probably less. The cost of Act 3, would likely be no different. :/

@paulirish

The JS execution costs are still the issue here so I profiled again using the dedicated JS profiler, mostly so we can inspect the tree view.

First, the initial flame chart:

image
~30s this time.

Also you can see the spikes at the end. That's the resize listener triggering from me scrolling up and down (which changes the omnibox's visibility (which changes the viewport size (which trigger a resize event))). :)

The tree view

I personally am not seeing any low-hanging fruit here, but I'm also less experienced in profiling react apps.
image

One thing that is clear is much of the cost is in the recursive component mounting that we saw in the tall stacks earlier:
image

@paulirish

Last effort to dig into the react work, I build the app and implemented instrumentation for React Perf (https://facebook.github.io/react/docs/perf.html).

The I measured from the start of horse-react's first render() call to the end of the first Listing component resize event.

Perf.printInclusive()

Prints the overall time taken.

image

Perf.printWasted(measurements)

The most useful part of the profiler.
"Wasted" time is spent on components that didn't actually render anything, e.g. the render stayed the same, so the DOM wasn't touched.

image

Perf.printExclusive()

"Exclusive" times don't include the times taken to mount the components: processing props, getInitialState, call componentWillMount and componentDidMount, etc.

image

Update: I've dumped these tables into this jsbin, which are probably more useful than a screenshot. ;)

@paulirish

Impressions from the React Perf output

The immediate thing I see is that all the printWasted numbers match the printInclusive ones. I suppose that effectively means "we nailed the serverside render so perfectly that React clientside agrees the DOM needs no changes." Which is pretty awesome. Of course it also means all time spent in render() is a no-op.

Moving to the printExclusive table, I think we have more useful numbers here, but please holler with whats standing out to you.

  • we have 3.2s worth of exclusive render() time which ends up being a bunch of no-op.
  • we have ~16.8s total of exclusive mount time.

I ended up dumping this table into Google Sheets to look at mount costs a little bit clearer.

Component mount costs

with sparklines. woo.
image

  • 60% (10 seconds) of mount costs are from icon components.
  • 1/3 of all mount costs come from the SnooIcon components. Those icons have more mount-time-per-instance than the other icons, plus theres >200 of 'em.
    • Looking at a common case, more than half of the snoo icons are offscreen in the sidenav. And the interactivity of the icons I haven't noticed while using the site recently. There may be an opportunity here to simplify with non-svg assets that don't go through the react component lifecycle.
  • The MobileButton component is also interesting. It appears to mostly function as a proper <a href> link (source). It's mounting costs are fairly high, but it doesn't seem to deliver that much value in return.
@paulirish

Summary and recommendations

The core UX concern here is that the page is unresponsive to user input for up to 17 seconds while it's loading. As interacting with reddit means bouncing to links and coming back, the "loading" process will actually happen quite often.

I think there are a few things that can be done to improve the responsiveness of the app as its loading.

Implementation Recommendations

  1. Reduce the size of the client.js payload. In #247 (comment) I found a few cases where we had more items in the bundle than expected. E.g. Dedupe the multiple lodash's, nuke dom-serializer.
    • Potential savings: 1-5 seconds till first render, 3-7 seconds of execution in Act 2.
  2. Shortcut the amount of work done in resize handlers. On Chrome and Safari, the address bar will scroll out of way, triggering resize. So a resize on mobile is pretty much inevitable for nearly all m.reddit visits.
    • Potential savings: ~2s of input-blocking JS execution.
  3. Simplify the SnooIcon component. Likely into something out of the react component lifecycle
    • Potential savings: ~7s of input-blocking JS execution.
  4. Remove the MobileButton component. It'd be better to keep that just a regular plain old link, if possible.
    • Potential savings: ~3s of execution. Tapping on links should also work immediately, as well, instead of waiting for pending JS to finish.
  5. Consider lazy instantiating the sidenav, perhaps on demand.
    • Potential savings: Potentially 50% of all component costs are in the sidenav, so quite a lot!
  6. Reconsider the animations. As the GSAP&SVG animations are JS based, they will only be performant if the main thread is not busy doing other work. During load there's basically no chance of that. But certainly later on (like loading comments), there probably is. You could easily measure how smooth your GSAP animations are in the wild.
    • Potential savings: Hard to say.
  7. Put the images for each post into the initial markup. <img> tag has higher priority than bg image, but both should be good enough.
    • Potential savings: 5 to 40 seconds until the user sees them

Perf ideas

Just a few ideas on staying on top of perf:

  1. Beacon some of these timings from React back to the server, to better see what perf in the wild looks like. The initial render() in particular. You can use performance.mark()/measure() (from the User Timing API) to safely get high-res timestamps.
  2. When looking at performance, always remote debug a real phone.. you can't get realistic numbers just with emulation on desktop.
  3. And try using a slightly shittier phone in your pocket for a week. :)

thx

Thanks for letting me dig into the project and have some fun here. ❤️ It's totally awesome that the site is open source -- that enabled me to get a much better understanding of things, build it on my own, try some modifications out locally and plug into other tools like disc. Such goodness. :)

I'll definitely be checking in here if you guys have follow up questions or ideas... or are seeing other opportunities based on your own analysis. And I've already started editing the above comments to add additional notes and links.

@paulirish paulirish changed the title from Loading performance: 45 seconds to load to Perf audit: Loading performance Jul 3, 2015
@paulirish

On twitter @petehunt, noted two things when it came to the component initialization:

This looks suspicious to me: https://github.com/reddit/reddit-mobile/blob/324c795357051db6a6af014638aa8ffdd35019d1/src/views/components/icons/SnooIcon.jsx#L91

  componentDidMount() {
    var svg = React.findDOMNode(this);
    this._scale = Math.min(svg.offsetWidth, svg.offsetHeight) / SVG.ICON_SIZE; // <<<<
  }

also this probably shouldn't live in state https://github.com/reddit/reddit-mobile/blob/1655d822c6f89bc490acb2d025fcfdd309950724/src/views/components/MobileButton.jsx#L83

  componentDidMount() {
    this.setState({touch: Utils.touch()});   // <<<<
  }

Mr @bringking notes that much of the SnooIcon code is for a mousemove effect.

Jordan added this could be packaged into a separate bundle to download separately. Perhaps only retrieved for hover capable devices.

@bringking

screen shot 2015-07-03 at 2 43 15 pm

Currently it seems SVG's not being rendered server side

https://github.com/reddit/reddit-mobile/blob/master/src/views/components/SVG.jsx#L53

which could explain why render is running on load for all the SnooIcons. Is there a reason for not rendering the SVG's on the server?

@MylesBorins

@paulirish as this is a browserify project, and you can use the --standalone flag to build standalone bundles, would you imagine breaking the payload up may result in a better experience? While you will not be able to do the same degree of dead code elimination if you include an entire lib, you might be able to even break apart some of the bundles of your own code.

For optimizing dedupe / dead code elimination you may want to introduce the uglifyify transform. Browserified bundles + uglify dead code elimination don't always play so nice together, depending on the project I have seen some pretty nice optimization for a one liner in the package.json

Perhaps playing with npm@3 and the flat dep tree could help solve this too.

@benjamingr

I wonder - why isn't the initial rendered HTML served from the server using React? It's one of its main selling points after all.

@kentaromiura

For lodash as a regular dependency:
requiring the full lodash alongside of ES2015 seems like a code smell:

grep -r "lodash" .

./assets/js/client.es6.js:import _ from 'lodash';
./buildTasks/icons.js: .pipe(consolidate('lodash', {
./buildTasks/js.js: ignore: /.+node_modules/(moment|q|react|reddit-textjs|superagent|gsap|lodash)/.+/i,
./buildTasks/utils/notify.js:var _ = require('lodash');
./package.json: "lodash": "3.6.0",
./src/views/components/Ad.jsx:import _ from 'lodash';
./src/views/components/ListingList.jsx:import _ from 'lodash';
./src/views/components/SearchBar.jsx:import _ from 'lodash';
./src/views/pages/submit.jsx:import _ from 'lodash';

https://github.com/reddit/reddit-mobile/blob/master/assets/js/client.es6.js#L146-L150

is equivalent of

Object.keys(config).forEach((key) => {
  bootstrap[key] && (config[key] = bootstrap[key])
})

https://github.com/reddit/reddit-mobile/blob/master/assets/js/client.es6.js#L302-L308
For the throttle function you don't need the whole lodash, so you could either require only lodash-throttle or using one of the many implementation available https://www.npmjs.com/search?q=throttle,
Btw binding app here makes no sense, so you should remove that

Same for debounce
https://github.com/reddit/reddit-mobile/blob/master/src/views/components/SearchBar.jsx#L24
https://www.npmjs.com/search?q=debounce

I'll just skip over the buildTask folder as the name imply is a server side step so no relevant
but suggest to move lodash on the devDependencies as after a couple more changes you will not need it anymore :)

https://github.com/reddit/reddit-mobile/blob/master/src/views/components/Ad.jsx#L152
You can rewrite this as

var listing = {}
Object.assign(listing, this.state.ad, { compact: props.compact });

https://github.com/reddit/reddit-mobile/blob/master/src/views/components/ListingList.jsx#L106-L108

var srnames = this.props.listings.reduce((prev, curr) => {
  if(!~prev.indexOf(curr.subreddit)) prev.push(curr.subreddit)
  return prev
}, []);
@CMDann
CMDann commented Jul 4, 2015

👍

@mattdesl
mattdesl commented Jul 4, 2015

lodash3 is modular, so you can require individual functions. Or install individual packages.

Also, browserify has a couple plugins for bundle splitting, which may be useful to deliver libs and load on demand:
https://www.npmjs.com/package/partition-bundle.
https://www.npmjs.com/package/factor-bundle

@NeXTs
NeXTs commented Jul 4, 2015

Comprehensive audit 👍

@ajacksified
Contributor

Thanks for the audit! Our plan is to tackle performance once we finish up a couple small things to bring us up to parity with the old mobile site. This helps make the case more obvious :D

A lot of this was very quickly built up from a tech demo, so it leaves quite a bit to be desired (and refactored). We're considering webpack for partial js loads, and more reduction of dependencies; beyond that, we clearly need to tune our React mounting. We've got some CDN fixes in testing that will get the initial load times reasonable, but that won't help with all the js.

@bringking - for the benefit of older phones (android 2.3, opera mini) that don't have SVG support; we disable svg on the server so that older phones can use non-svg, and if you have svg, it's enabled on top. One way we could accomplish both without a re-render is to render both, the SVG on top; and if SVG isn't supported, you see the icon behind it.

@benjamingr It is.

@kentaromiura / @mattdesl - it's actually mostly used for some of our dependencies, and crept into the main project since it was loaded anyway. I've been meaning to go through and refactor it out of some of them (which generally each only use a function or two), but I haven't had time to submit PRs to each of them yet.

@paulirish - you're awesome. Thanks for putting the work into this, both for us and to share.

@jdalton
jdalton commented Jul 4, 2015

@ajacksified
As an example of what @mattdesl mentioned you can cherry-pick methods in lodash and webpack/browserify will tree shake them:

var debounce = require('lodash/function/debounce');
var uniq = require('lodash/array/uniq');

Or use one of the individual packages:

var debounce = require('lodash.debounce');
var uniq = require('lodash.uniq');
@ajacksified
Contributor

Yup! I'm aware, just need to refactor it out of my deps.
On Jul 4, 2015 9:34 AM, "John-David Dalton" notifications@github.com
wrote:

@ajacksified https://github.com/ajacksified
As an example of what @mattdesl https://github.com/mattdesl mentioned
you can cherry-pick methods in lodash and webpack/browserify will tree
shake them:

var debounce = require('lodash/function/debounce');

Or use one of the individual packages
https://www.npmjs.com/browse/keyword/lodash-modularized:

var debounce = require('lodash.debounce');


Reply to this email directly or view it on GitHub
#247 (comment)
.

@jdalton
jdalton commented Jul 4, 2015

Yup! I'm aware, just need to refactor it out of my deps.

I think leveraging its modules is better than refactoring out or reimplementing one-off inline alternatives. The whole win is knowing you have something that's readable, tested, consistent, performs well, yada yada, without having to reimplement/reinvent anything as modules remove the size concern.

@mattdesl
mattdesl commented Jul 4, 2015

BTW - if the only thing you are using greensock for is DrawSVGPlugin, you might want to check out this project instead:

https://github.com/maxwellito/vivus/

Still a bit bloated, but 10kb is better than 100kb.

@Azeirah
Azeirah commented Jul 4, 2015

Another optimization you could try is running the google closure dead-code elimination tool.

@ajacksified
Contributor

Oh, right- specifically, what I meant was to change "require lodash" to
"require lodash/whatever" in my deps, and then update all of my deps' deps
to point at the git shas until they accept prs and update versions. The
main holdup is that I have to fix the whole tree, unless there's npm magic
I'm not aware of.
On Jul 4, 2015 9:48 AM, "John-David Dalton" notifications@github.com
wrote:

Yup! I'm aware, just need to refactor it out of my deps.

I think leveraging its modules is better than refactoring out or
reimplementing one-off inline alternatives. The whole win is knowing you
have something that's readable, tested, consistent, performs well, yada
yada.


Reply to this email directly or view it on GitHub
#247 (comment)
.

@MylesBorins

@jdalton is them :p

@framerate

This is amazing. @paulirish next time you're in Los Angeles come do this to our code :)

I recently switched to webpack from browserify and used their built in dedupe and optimization plugins. I'd be interested to see the the output size difference if anyone has run them side by side.

@killercup

Wow, that's an awesome audit.

Indeed, client.min.js is 1.1MB of minified code.

Looking at your build tasks, it seems you are including React in your browserify build and minify it yourself. You should probably use envify and set NODE_ENV='production' (as noted here) to omit unnecessary code (IIRC, this will strip perf tools, though). In my experience, using the same settings, webpack produces even less code than browserify + envify + uglify.

@kentaromiura

unless there's npm magic I'm not aware of.

You mean something like https://docs.npmjs.com/cli/dedupe ?

@jordwalke
  1. The best way to decrease time spent in rendering (JS/React) is to render fewer feed units. It looks like you may be rendering as many as 20 on the first page load. For Facebook, we'll often only render three, then fetch the data for several more after the first three have been rendered. For example, if you're spending 10 seconds rendering 20 feed units, rendering only three could cut that down to as low as 1.5 seconds - which is still far too long based on my experience so there's likely something going on in the rendering code as well. Either way, 1.5s is much better than 10s!

  2. When scrolling, I recommend never rendering more than a single feed unit per event loop. React Native has a <ListView> that ensures this. I suggest you poke around and build/use something similar. In my experience, individual stories of this complexity should be able to be rendered (JS time) in about 16ms. Let me know if you'd like more suggestions to getting there. Make sure your abstraction ensures that shouldComponentUpdate returns false on any unit that you know didn't change (for example, if you load two more rows, make sure all the previous rows don't update).

  3. I've confirmed that you are also rendering all of the menu content even though it's not shown. I've made this same mistake in Facebook's client rendered menus and doubled my app's rendering time inadvertently! It's such an easy mistake to make because they're out of sight.

  4. I haven't confirmed, but if all of those SVG icons are likely very expensive, accounting for a huge portion of rendering time. There's a bunch you can do to optimize them, but one really quick trick is to pull out the constant parts of the SVG into the top of your module definition, and just return those constants from render(). For example, in SeaShellIcon.jsx, this portion can be pulled up to the top of the file:

      var REUSABLE_SVG =
          <SVG className='SeashellIcon' fallbackIcon='icon-seashells'>
            <g className='SVG-fill'>
              <circle ref="one" cx={SVG.ICON_SIZE / 2 - _DIST} cy={SVG.ICON_SIZE / 2} r={_DIAMETER}/>
              <circle cx={SVG.ICON_SIZE / 2} cy={SVG.ICON_SIZE / 2} r={_DIAMETER}/>
              <circle ref="three" cx={SVG.ICON_SIZE / 2 + _DIST} cy={SVG.ICON_SIZE / 2} r={_DIAMETER}/>
            </g>
          </SVG>;

So I imagine that you should be able to get the client (JS) rendering time down to about 50ms for the first few stories, and under 20ms for each subsequent one.

@ajacksified: I do agree with your priorities - build it first, optimize render paths later. Please let us know how we can help.

@padsbanger

very interesting audit 👍

@mr47
mr47 commented Jul 5, 2015

@paulirish an amazing reactJS review, may be create a post about reactJS perfomance? Because it seems like Reddit team just use reactJS and says let use it everywhere and don't know about the "weak sides" of react.

Thanks!

@ajacksified
Contributor

@kentaromiura - I have npm dependencies that have require('lodash'), so dedupe won't help me there - I still have to modify my npm dependencies to require('lodash/thing') otherwise they'll still pull in all of lodash.

@killercup - thanks for the suggestions!

For Facebook, we'll often only render three, then fetch the data for several more after the first three have been rendered.

One of mobile web's challenges is dealing with a very slow API, so we end up having to get a larger batch of data up front. However, we can try the render-only-a-few-at-a-time trick, which I imagine will help. Good suggestions on not rendering hidden stuff (like the menu) until click, and pulling common files up to the top.

I also intend do some investigation on our SVG use; rendering only as elements come into view will help out there, but we might have to be more aggressive to see which phones we should even present SVG to.

@derblub
derblub commented Jul 5, 2015

👍

@jordwalke

One of mobile web's challenges is dealing with a very slow API, so we end up having to get a larger batch of data up front. However, we can try the render-only-a-few-at-a-time trick, which I imagine will help.

This is what the list view does, among other things. The amount of data you have, should remain independent from the amount of data rendered.

@ajacksified
Contributor

@jordwalke yup, that makes sense. I'm going to play around with an implementation of that.

@ajacksified ajacksified referenced this issue Jul 6, 2015
Merged

Performance #249

@cto
cto commented Jul 6, 2015

Interesting Paul,
Is there a similar perf. audit. tool that is plugable into Jenkins ?

@Jakobud
Jakobud commented Jul 6, 2015

@bringking

Is there a reason for not rendering the SVG's on the server?

What are you talking about? SVG is just ascii.

@benjamingr

I think @bringking is talking about all the SVG components they're
rendering with react. Like
https://github.com/reddit/reddit-mobile/blob/master/src/views/components/icons/GoldIcon.jsx
.

On Mon, Jul 6, 2015 at 6:55 PM, Jake Wilson notifications@github.com
wrote:

@bringking https://github.com/bringking

Is there a reason for not rendering the SVG's on the server?

What are you talking about? SVG is just ascii.


Reply to this email directly or view it on GitHub
#247 (comment)
.

@ajacksified
Contributor

@Jakobud - @bringking is asking why the server is rendering the fallback for non-svg browsers, rather than rendering the markup for the svgs.

@bringking

@Jakobud @benjamingr, @ajacksified is correct. I was asking what the rational is for falling back to a

icon-font on the initial server render, thus causing a discrepancy in the initial client side render pass in most modern browsers. @ajacksified already addressed the rational for older device support. Relevant line is here - https://github.com/reddit/reddit-mobile/blob/master/src/views/components/SVG.jsx#L53

@azizhk
azizhk commented Jul 7, 2015

Should they split up their 1.1 mb JS file?
Even Google PageSpeed Module recommends not keeping JS greater than 90kb
https://developers.google.com/speed/pagespeed/module/filter-js-combine

Also every time they are going to deploy the website, a new version of the script would be generated and nothing gets cached for existing users.
You can split it into varying levels of churn rate.

  • Core Third Party tools which are going to be updated in like 6 months.
  • Modules which get used at multiple places but not everywhere.
  • Page specific JS.
    Reddit being a site where every page is the same, this logic cannot be directly applied but something similar can be used.
@danehansen

@jordwalke

...but one really quick trick is to pull out the constant parts of the SVG into the top of your module definition, and just return those constants from render(). For example, in SeaShellIcon.jsx, this portion can be pulled up to the top of the file...

I am intrigued by this, but confused as to what it is actually accomplishing. Is this because our object returned in render is only part of the pseudo DOM, and therefore it can be reused across many instances that coexist on the page?

@jordwalke

Is this because our object returned in render is only part of the pseudo DOM, and therefore it can be reused across many instances that coexist on the page?

@danehansen Exactly! The fact that it is never mutated, and never changes means it can be pulled up to the top and shared across all invocations and instances. That saves on allocations among other things. Now that you mention it, I'm not sure if this version of React supports that yet (Sebastian has been working on making this possible - by cleaning up mistakes that I've made in the earliest versions of the React core!). It's worth a shot though. Let me know if you have problems doing this.

@ericlaw1979

Beyond all the very cool items above, you can also easily improve load time of the site by improving use of compression: use Zopfli on static assets.

For instance, https://www.redditstatic.com/mobile-web/js/client.min-1c3a5bb7.js is 295,104 bytes but shrinks to 269,233 bytes when compressed with Zopfli.

The images downloaded by default:
https://www.redditstatic.com/mobile-web/favicon/192x192.png
https://www.redditstatic.com/mobile-web/favicon/128x128.png
https://www.redditstatic.com/mobile-web/favicon/64x64.png
...each save about 7% of size when their IDAT sections are compressed with Zopfli. ( See http://textslashplain.com/2015/06/16/optimize-pngs-with-pngdistill/ )

@jdalton
jdalton commented Jul 8, 2015

@ericlaw1979 I ❤️ Zopfli!

Have y'all tried compressing the images with other utils like ImageOptim /@pornel

@danehansen

@jordwalke

Exactly! The fact that it is never mutated, and never changes means it can be pulled up to the top and shared across all invocations and instances. That saves on allocations among other things. Now that you mention it, I'm not sure if this version of React supports that yet (Sebastian has been working on making this possible - by cleaning up mistakes that I've made in the earliest versions of the React core!). It's worth a shot though. Let me know if you have problems doing this.

Looks like refs can't be handled outside of a render function, which cuts down the areas I could implement this by quite a bit. I dont know enough about how rendering of JSX works... would it be beneficial to define unchanging chunks within a parent JSX object as constants?

@jordwalke

@danehansen It wouldn't hurt! But it's also likely that the majority of component rendering time can easily be recovered by simply doing something very manual/hacky/imperative. That's what we do when we build with React. We implement everything as elegantly as possible to help us get a shippable product, measure the bottlenecks, then special case a handful of things.

@zanedev
zanedev commented Jul 10, 2015

Good stuff thanks! Shows how much extra work has to go into performance tuning especially on mobile devices.

@greenido

Great work @paulirish - Very interesting to follow your 'path' and see what are the issues we wish to look at!

@briandipalma

@danehansen Some of optimizations @jordwalke mentioned might be done automatically by Babel if you enable support for them, more details here

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